| Message ID | 20230412145053.114847-6-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add RZ/G2L SCIFA DMAC/ SCI TX support | expand |
Hi Biju, On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > We need to set TE to "0" (i.e., disable serial transmission) to > get the expected behavior of the end of serial transmission. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port *port) > } else if (!uart_circ_empty(xmit) && !stopped) { > c = xmit->buf[xmit->tail]; > xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > + } else if (port->type == PORT_SCI && uart_circ_empty(xmit)) { > + ctrl = serial_port_in(port, SCSCR); > + ctrl &= ~SCSCR_TE; > + serial_port_out(port, SCSCR, ctrl); > + return; So nothing after the while loop should be done anymore? What about clearing SCSCR_TE in sci_stop_tx() (which is called after the while loop) instead? > } else { > break; > } So combined with my comments on patch 4/5, that would become - if (port->type == PORT_SCI) + if (port->type == PORT_SCI) { ctrl |= SCSCR_TEIE; + ctrl &= ~SCSCR_TE; + } in sci_stop_tx(). 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
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Friday, April 21, 2023 10:50 AM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby > <jirislaby@kernel.org>; Yoshinori Sato <ysato@users.sourceforge.jp>; linux- > serial@vger.kernel.org; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org > Subject: Re: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on > SCI > > Hi Biju, > > On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > We need to set TE to "0" (i.e., disable serial transmission) to get > > the expected behavior of the end of serial transmission. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port > *port) > > } else if (!uart_circ_empty(xmit) && !stopped) { > > c = xmit->buf[xmit->tail]; > > xmit->tail = (xmit->tail + 1) & > > (UART_XMIT_SIZE - 1); > > + } else if (port->type == PORT_SCI && > uart_circ_empty(xmit)) { > > + ctrl = serial_port_in(port, SCSCR); > > + ctrl &= ~SCSCR_TE; > > + serial_port_out(port, SCSCR, ctrl); > > + return; > > So nothing after the while loop should be done anymore? > What about clearing SCSCR_TE in sci_stop_tx() (which is called after the > while loop) instead? > > > } else { > > break; > > } > > So combined with my comments on patch 4/5, that would become > > - if (port->type == PORT_SCI) > + if (port->type == PORT_SCI) { > ctrl |= SCSCR_TEIE; > + ctrl &= ~SCSCR_TE; > + } > > in sci_stop_tx(). I get stray chars at the end and Carriage return not happening. Basically tx is not working as expected. Cheers, Biju
On Fri, 21 Apr 2023, Geert Uytterhoeven wrote: > Hi Biju, > > On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > We need to set TE to "0" (i.e., disable serial transmission) to > > get the expected behavior of the end of serial transmission. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port *port) > > } else if (!uart_circ_empty(xmit) && !stopped) { > > c = xmit->buf[xmit->tail]; > > xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > > + } else if (port->type == PORT_SCI && uart_circ_empty(xmit)) { > > + ctrl = serial_port_in(port, SCSCR); > > + ctrl &= ~SCSCR_TE; > > + serial_port_out(port, SCSCR, ctrl); > > + return; > > So nothing after the while loop should be done anymore? > What about clearing SCSCR_TE in sci_stop_tx() (which is called after > the while loop) instead? Yes, this added fragment doesn't really seem to belong into the tx loop. The right direction would be to aim towards converting the whole tx loop into using uart_port_tx_limited().
> -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Friday, April 21, 2023 1:54 PM > To: Geert Uytterhoeven <geert@linux-m68k.org>; Biju Das > <biju.das.jz@bp.renesas.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby > <jirislaby@kernel.org>; Yoshinori Sato <ysato@users.sourceforge.jp>; linux- > serial <linux-serial@vger.kernel.org>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org > Subject: Re: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on > SCI > > On Fri, 21 Apr 2023, Geert Uytterhoeven wrote: > > > Hi Biju, > > > > On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > We need to set TE to "0" (i.e., disable serial transmission) to get > > > the expected behavior of the end of serial transmission. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/tty/serial/sh-sci.c > > > +++ b/drivers/tty/serial/sh-sci.c > > > @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port > *port) > > > } else if (!uart_circ_empty(xmit) && !stopped) { > > > c = xmit->buf[xmit->tail]; > > > xmit->tail = (xmit->tail + 1) & > > > (UART_XMIT_SIZE - 1); > > > + } else if (port->type == PORT_SCI && > uart_circ_empty(xmit)) { > > > + ctrl = serial_port_in(port, SCSCR); > > > + ctrl &= ~SCSCR_TE; > > > + serial_port_out(port, SCSCR, ctrl); > > > + return; > > > > So nothing after the while loop should be done anymore? > > What about clearing SCSCR_TE in sci_stop_tx() (which is called after > > the while loop) instead? > > Yes, this added fragment doesn't really seem to belong into the tx loop. The difference is, Maybe this IP doesn't have FIFO compared to other SCIF IPs. and Carriage return is not handled properly without this fix. > The right direction would be to aim towards converting the whole tx loop > into using uart_port_tx_limited(). But looks like this change will have bigger impact as we need to test wide range of SoCs in both interrupt and DMA mode. Cheers, Biju
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 5c9ae69c0555..7c9457962a3d 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -847,6 +847,11 @@ static void sci_transmit_chars(struct uart_port *port) } else if (!uart_circ_empty(xmit) && !stopped) { c = xmit->buf[xmit->tail]; xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); + } else if (port->type == PORT_SCI && uart_circ_empty(xmit)) { + ctrl = serial_port_in(port, SCSCR); + ctrl &= ~SCSCR_TE; + serial_port_out(port, SCSCR, ctrl); + return; } else { break; }
We need to set TE to "0" (i.e., disable serial transmission) to get the expected behavior of the end of serial transmission. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v3->v4: * Updated commit description. v3: New patch. split from patch#3. --- drivers/tty/serial/sh-sci.c | 5 +++++ 1 file changed, 5 insertions(+)