[PATCH v4 0/5] Add RZ/G2L SCIFA DMAC/ SCI TX support

[PATCH v4 0/5] Add RZ/G2L SCIFA DMAC/ SCI TX support

[Biju Das]

This patch series aims to add DMA support for SCIFA/ SCI TX support on
RZ/G2L alike SoCs.

v3->v4:
 * Updated commit description for patch#1,#2,#3 and #5
 * Removed fixes tag from patch#3
v2->v3:
 * Dropped inline function is_rz_scif_port() and instead using
   s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE for finding RZ SCIF port.
 * Dropped SCIx_TEI_IRQ as DMAC activation not possible with TEI interrupt.
 * Updated the code flow similar to SCIFA/SCIFB DMAC tx handling.
 * Replaced is_rz_scif_port to s->cfg->regtype check
 * Updated the code flow similar to SCIFA/SCIFB DMA rx handling.
 * Added remaining patches from Renesas SCI fixes patch series to this series.
 * Updated commit description for patch#3
 * Moved handling of clearing TE bit as separate patch#5.
v1->v2:
 * Added support for DMA tx and rx.

Biju Das (5):
  tty: serial: sh-sci: Add RZ/G2L SCIF DMA tx support
  tty: serial: sh-sci: Add RZ/G2L SCIF DMA rx support
  tty: serial: sh-sci: Fix TE setting on SCI IP
  tty: serial: sh-sci: Add support for tx end interrupt handling
  tty: serial: sh-sci: Fix end of transmission on SCI

 drivers/tty/serial/sh-sci.c | 96 +++++++++++++++++++++++++++++++------
 drivers/tty/serial/sh-sci.h |  3 ++
 2 files changed, 85 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx support

[Biju Das]

SCIFA IP on RZ/G2L SoC has the same signal for both interrupt
and DMA transfer request. Setting DMARS register for DMA transfer
makes the signal to work as a DMA transfer request signal and
subsequent interrupt requests to the interrupt controller
are masked. Similarly clearing DMARS register makes signal to work as
interrupt signal and subsequent interrupt requests to the interrupt
controller are unmasked.

Add SCIFA DMA tx support for RZ/G2L alike SoCs by disabling TXI line
interrupt and setting DMARS registers by DMA api for DMA transfer request.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Updated commit description by removing tx end interrupt.
v2->v3:
 * Dropped inline function is_rz_scif_port() and instead using
   s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE for finding RZ SCIF port.
 * Dropped SCIx_TEI_IRQ as DMAC activation not possible with TEI interrupt.
 * Updated the code flow similar to SCIFA/SCIFB DMAC tx handling.
v1->v2:
 * No change
---
 drivers/tty/serial/sh-sci.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ca31e34afd83..f70d06a03864 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -588,12 +588,17 @@ static void sci_start_tx(struct uart_port *port)
 
 	if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
 	    dma_submit_error(s->cookie_tx)) {
+		if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
+			/* Switch irq from SCIF to DMA */
+			disable_irq(s->irqs[SCIx_TXI_IRQ]);
+
 		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
 	}
 #endif
 
-	if (!s->chan_tx || port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
+	if (!s->chan_tx || s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE ||
+	    port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
 		/* Set TIE (Transmit Interrupt Enable) bit in SCSCR */
 		ctrl = serial_port_in(port, SCSCR);
 		serial_port_out(port, SCSCR, ctrl | SCSCR_TIE);
@@ -1192,9 +1197,15 @@ static void sci_dma_tx_complete(void *arg)
 		schedule_work(&s->work_tx);
 	} else {
 		s->cookie_tx = -EINVAL;
-		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
+		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
+		    s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
 			u16 ctrl = serial_port_in(port, SCSCR);
 			serial_port_out(port, SCSCR, ctrl & ~SCSCR_TIE);
+			if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
+				/* Switch irq from DMA to SCIF */
+				dmaengine_pause(s->chan_tx_saved);
+				enable_irq(s->irqs[SCIx_TXI_IRQ]);
+			}
 		}
 	}
 
-- 
2.25.1

Re: [PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx support

[Geert Uytterhoeven]

Hi Biju,

On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> SCIFA IP on RZ/G2L SoC has the same signal for both interrupt
> and DMA transfer request. Setting DMARS register for DMA transfer
> makes the signal to work as a DMA transfer request signal and
> subsequent interrupt requests to the interrupt controller
> are masked. Similarly clearing DMARS register makes signal to work as
> interrupt signal and subsequent interrupt requests to the interrupt
> controller are unmasked.
>
> Add SCIFA DMA tx support for RZ/G2L alike SoCs by disabling TXI line
> interrupt and setting DMARS registers by DMA api for DMA transfer request.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3->v4:
>  * Updated commit description by removing tx end interrupt.

Thanks for the update!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -588,12 +588,17 @@ static void sci_start_tx(struct uart_port *port)
>
>         if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
>             dma_submit_error(s->cookie_tx)) {
> +               if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> +                       /* Switch irq from SCIF to DMA */
> +                       disable_irq(s->irqs[SCIx_TXI_IRQ]);

Please wrap this block inside curly braces.

> +

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

RE: [PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx support

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Friday, April 21, 2023 10:22 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx
> support
> 
> Hi Biju,
> 
> On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > SCIFA IP on RZ/G2L SoC has the same signal for both interrupt and DMA
> > transfer request. Setting DMARS register for DMA transfer makes the
> > signal to work as a DMA transfer request signal and subsequent
> > interrupt requests to the interrupt controller are masked. Similarly
> > clearing DMARS register makes signal to work as interrupt signal and
> > subsequent interrupt requests to the interrupt controller are
> > unmasked.
> >
> > Add SCIFA DMA tx support for RZ/G2L alike SoCs by disabling TXI line
> > interrupt and setting DMARS registers by DMA api for DMA transfer request.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3->v4:
> >  * Updated commit description by removing tx end interrupt.
> 
> Thanks for the update!
> 
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -588,12 +588,17 @@ static void sci_start_tx(struct uart_port *port)
> >
> >         if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
> >             dma_submit_error(s->cookie_tx)) {
> > +               if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> > +                       /* Switch irq from SCIF to DMA */
> > +                       disable_irq(s->irqs[SCIx_TXI_IRQ]);
> 
> Please wrap this block inside curly braces.

I thought, for single if statement braces not needed.
That is why I haven't added. Am I missing anything here?

Note:
This patch is already applied on tty-next. I need to send a separate patch, if braces is required.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=8749061be196b41a874d71c073c03171bf2741b2

Cheers,
Biju

> 
> > +
> 
> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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

Re: [PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx support

[Geert Uytterhoeven]

Hi Biju,

On Fri, Apr 21, 2023 at 11:56 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Friday, April 21, 2023 10:22 AM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> > <jirislaby@kernel.org>; linux-serial@vger.kernel.org; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> > lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> > Subject: Re: [PATCH v4 1/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA tx
> > support
> >
> > Hi Biju,
> >
> > On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > SCIFA IP on RZ/G2L SoC has the same signal for both interrupt and DMA
> > > transfer request. Setting DMARS register for DMA transfer makes the
> > > signal to work as a DMA transfer request signal and subsequent
> > > interrupt requests to the interrupt controller are masked. Similarly
> > > clearing DMARS register makes signal to work as interrupt signal and
> > > subsequent interrupt requests to the interrupt controller are
> > > unmasked.
> > >
> > > Add SCIFA DMA tx support for RZ/G2L alike SoCs by disabling TXI line
> > > interrupt and setting DMARS registers by DMA api for DMA transfer request.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v3->v4:
> > >  * Updated commit description by removing tx end interrupt.
> >
> > Thanks for the update!
> >
> > > --- a/drivers/tty/serial/sh-sci.c
> > > +++ b/drivers/tty/serial/sh-sci.c
> > > @@ -588,12 +588,17 @@ static void sci_start_tx(struct uart_port *port)
> > >
> > >         if (s->chan_tx && !uart_circ_empty(&s->port.state->xmit) &&
> > >             dma_submit_error(s->cookie_tx)) {
> > > +               if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
> > > +                       /* Switch irq from SCIF to DMA */
> > > +                       disable_irq(s->irqs[SCIx_TXI_IRQ]);
> >
> > Please wrap this block inside curly braces.
>
> I thought, for single if statement braces not needed.
> That is why I haven't added. Am I missing anything here?

Technically, it's indeed a single statement.  But combined with the comment,
it spans multiple lines.

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

[PATCH v4 2/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA rx support

[Biju Das]

SCIFA IP on RZ/G2L SoC has the same signal for both interrupt
and DMA transfer request. Setting DMARS register for DMA transfer
makes the signal to work as a DMA transfer request signal and
subsequent interrupt requests to the interrupt controller
are masked. Similarly clearing DMARS register makes signal to work as
interrupt signal and subsequent interrupt requests to the interrupt
controller are unmasked.

Add SCIFA DMA rx support for RZ/G2L alike SoCs by disabling RXI line
interrupt and setting DMARS registers by DMA api for DMA transfer request.

Apart from this, we must set FIFO trigger to 1 for the expected behavior
of the receive transmission.

While at it replace the parameter irq to s->irqs[SCIx_RXI_IRQ] in
disable_irq_nosync() to match enable_irq() in sci_dma_rx_reenable_irq().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->4:
 * Updated commit description.
v2->v3:
 * Replaced is_rz_scif_port to s->cfg->regtype check
 * Updated the code flow similar to SCIFA/SCIFB DMA rx handling.
v2:
 * New patch
---
 drivers/tty/serial/sh-sci.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index f70d06a03864..15743c2f3d3d 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1277,9 +1277,13 @@ static void sci_dma_rx_reenable_irq(struct sci_port *s)
 
 	/* Direct new serial port interrupts back to CPU */
 	scr = serial_port_in(port, SCSCR);
-	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
-		scr &= ~SCSCR_RDRQE;
+	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
+	    s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
 		enable_irq(s->irqs[SCIx_RXI_IRQ]);
+		if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
+			scif_set_rtrg(port, s->rx_trigger);
+		else
+			scr &= ~SCSCR_RDRQE;
 	}
 	serial_port_out(port, SCSCR, scr | SCSCR_RIE);
 }
@@ -1516,7 +1520,8 @@ static enum hrtimer_restart sci_dma_rx_timer_fn(struct hrtimer *t)
 			tty_flip_buffer_push(&port->state->port);
 	}
 
-	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
+	if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
+	    s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
 		sci_dma_rx_submit(s, true);
 
 	sci_dma_rx_reenable_irq(s);
@@ -1640,7 +1645,8 @@ static void sci_request_dma(struct uart_port *port)
 
 		s->chan_rx_saved = s->chan_rx = chan;
 
-		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB)
+		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
+		    s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE)
 			sci_dma_rx_submit(s, false);
 	}
 }
@@ -1693,9 +1699,15 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 		u16 ssr = serial_port_in(port, SCxSR);
 
 		/* Disable future Rx interrupts */
-		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
-			disable_irq_nosync(irq);
-			scr |= SCSCR_RDRQE;
+		if (port->type == PORT_SCIFA || port->type == PORT_SCIFB ||
+		    s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
+			disable_irq_nosync(s->irqs[SCIx_RXI_IRQ]);
+			if (s->cfg->regtype == SCIx_RZ_SCIFA_REGTYPE) {
+				scif_set_rtrg(port, 1);
+				scr |= SCSCR_RIE;
+			} else {
+				scr |= SCSCR_RDRQE;
+			}
 		} else {
 			if (sci_dma_rx_submit(s, false) < 0)
 				goto handle_pio;
-- 
2.25.1

Re: [PATCH v4 2/5] tty: serial: sh-sci: Add RZ/G2L SCIFA DMA rx support

[Geert Uytterhoeven]

On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> SCIFA IP on RZ/G2L SoC has the same signal for both interrupt
> and DMA transfer request. Setting DMARS register for DMA transfer
> makes the signal to work as a DMA transfer request signal and
> subsequent interrupt requests to the interrupt controller
> are masked. Similarly clearing DMARS register makes signal to work as
> interrupt signal and subsequent interrupt requests to the interrupt
> controller are unmasked.
>
> Add SCIFA DMA rx support for RZ/G2L alike SoCs by disabling RXI line
> interrupt and setting DMARS registers by DMA api for DMA transfer request.
>
> Apart from this, we must set FIFO trigger to 1 for the expected behavior
> of the receive transmission.
>
> While at it replace the parameter irq to s->irqs[SCIx_RXI_IRQ] in
> disable_irq_nosync() to match enable_irq() in sci_dma_rx_reenable_irq().
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3->4:
>  * Updated commit description.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v4 3/5] tty: serial: sh-sci: Fix TE setting on SCI IP

[Biju Das]

As per the RZ/G2L users hardware manual (Rev.1.20 Sep, 2022), section
23.3.7 Serial Data Transmission (Asynchronous Mode) it is mentioned
that the TE (transmit enable) must be set after setting TIE (transmit
interrupt enable) or these 2 bits are set to 1 simultaneously by a
single instruction. So set these 2 bits in single instruction.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4:
 * Removed fixes tag.
v3:
 * New patch moved here from Renesas SCI fixes patch series
 * Updated commit description
 * Moved handling of clearing TE bit as separate patch#5.
---
 drivers/tty/serial/sh-sci.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 15743c2f3d3d..32f5c1f7d697 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -601,6 +601,15 @@ static void sci_start_tx(struct uart_port *port)
 	    port->type == PORT_SCIFA || port->type == PORT_SCIFB) {
 		/* Set TIE (Transmit Interrupt Enable) bit in SCSCR */
 		ctrl = serial_port_in(port, SCSCR);
+
+		/*
+		 * For SCI, TE (transmit enable) must be set after setting TIE
+		 * (transmit interrupt enable) or in the same instruction to start
+		 * the transmit process.
+		 */
+		if (port->type == PORT_SCI)
+			ctrl |= SCSCR_TE;
+
 		serial_port_out(port, SCSCR, ctrl | SCSCR_TIE);
 	}
 }
@@ -2600,8 +2609,14 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
 		sci_set_mctrl(port, port->mctrl);
 	}
 
-	scr_val |= SCSCR_RE | SCSCR_TE |
-		   (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0));
+	/*
+	 * For SCI, TE (transmit enable) must be set after setting TIE
+	 * (transmit interrupt enable) or in the same instruction to
+	 * start the transmitting process. So skip setting TE here for SCI.
+	 */
+	if (port->type != PORT_SCI)
+		scr_val |= SCSCR_TE;
+	scr_val |= SCSCR_RE | (s->cfg->scscr & ~(SCSCR_CKE1 | SCSCR_CKE0));
 	serial_port_out(port, SCSCR, scr_val | s->hscif_tot);
 	if ((srr + 1 == 5) &&
 	    (port->type == PORT_SCIFA || port->type == PORT_SCIFB)) {
-- 
2.25.1

Re: [PATCH v4 3/5] tty: serial: sh-sci: Fix TE setting on SCI IP

[Geert Uytterhoeven]

On Wed, Apr 12, 2023 at 4:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per the RZ/G2L users hardware manual (Rev.1.20 Sep, 2022), section
> 23.3.7 Serial Data Transmission (Asynchronous Mode) it is mentioned
> that the TE (transmit enable) must be set after setting TIE (transmit
> interrupt enable) or these 2 bits are set to 1 simultaneously by a
> single instruction. So set these 2 bits in single instruction.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v4:
>  * Removed fixes tag.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v4 4/5] tty: serial: sh-sci: Add support for tx end interrupt handling

[Biju Das]

As per the RZ/G2L users hardware manual (Rev.1.20 Sep, 2022), section
23.3.7 Serial Data Transmission (Asynchronous Mode), it is mentioned
that, set the SCR.TIE bit to 0 and SCR.TEIE bit to 1, after the last
data to be transmitted are written to the TDR.

This will generate tx end interrupt and in the handler set SCR.TE and
SCR.TEIE to 0.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * No change.
v3:
 * New patch. moved from Renesas SCI fixes patch series.
---
 drivers/tty/serial/sh-sci.c | 31 ++++++++++++++++++++++++++++---
 drivers/tty/serial/sh-sci.h |  3 +++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 32f5c1f7d697..5c9ae69c0555 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -860,9 +860,16 @@ static void sci_transmit_chars(struct uart_port *port)
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
-	if (uart_circ_empty(xmit))
-		sci_stop_tx(port);
+	if (uart_circ_empty(xmit)) {
+		if (port->type == PORT_SCI) {
+			ctrl = serial_port_in(port, SCSCR);
+			ctrl &= ~SCSCR_TIE;
+			ctrl |= SCSCR_TEIE;
+			serial_port_out(port, SCSCR, ctrl);
+		}
 
+		sci_stop_tx(port);
+	}
 }
 
 static void sci_receive_chars(struct uart_port *port)
@@ -1766,6 +1773,24 @@ static irqreturn_t sci_tx_interrupt(int irq, void *ptr)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t sci_tx_end_interrupt(int irq, void *ptr)
+{
+	struct uart_port *port = ptr;
+	unsigned long flags;
+	unsigned short ctrl;
+
+	if (port->type != PORT_SCI)
+		return sci_tx_interrupt(irq, ptr);
+
+	spin_lock_irqsave(&port->lock, flags);
+	ctrl = serial_port_in(port, SCSCR);
+	ctrl &= ~(SCSCR_TE | SCSCR_TEIE);
+	serial_port_out(port, SCSCR, ctrl);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t sci_br_interrupt(int irq, void *ptr)
 {
 	struct uart_port *port = ptr;
@@ -1902,7 +1927,7 @@ static const struct sci_irq_desc {
 
 	[SCIx_TEI_IRQ] = {
 		.desc = "tx end",
-		.handler = sci_tx_interrupt,
+		.handler = sci_tx_end_interrupt,
 	},
 
 	/*
diff --git a/drivers/tty/serial/sh-sci.h b/drivers/tty/serial/sh-sci.h
index c0ae78632dda..0b65563c4e9e 100644
--- a/drivers/tty/serial/sh-sci.h
+++ b/drivers/tty/serial/sh-sci.h
@@ -59,6 +59,9 @@ enum {
 #define SCSMR_SRC_19	0x0600	/* Sampling rate 1/19 */
 #define SCSMR_SRC_27	0x0700	/* Sampling rate 1/27 */
 
+/* Serial Control Register, SCI only bits */
+#define SCSCR_TEIE	BIT(2)  /* Transmit End Interrupt Enable */
+
 /* Serial Control Register, SCIFA/SCIFB only bits */
 #define SCSCR_TDRQE	BIT(15)	/* Tx Data Transfer Request Enable */
 #define SCSCR_RDRQE	BIT(14)	/* Rx Data Transfer Request Enable */
-- 
2.25.1

Re: [PATCH v4 4/5] tty: serial: sh-sci: Add support for tx end interrupt handling

[Geert Uytterhoeven]

Hi Biju,

On Wed, Apr 12, 2023 at 4:57 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> As per the RZ/G2L users hardware manual (Rev.1.20 Sep, 2022), section
> 23.3.7 Serial Data Transmission (Asynchronous Mode), it is mentioned
> that, set the SCR.TIE bit to 0 and SCR.TEIE bit to 1, after the last
> data to be transmitted are written to the TDR.
>
> This will generate tx end interrupt and in the handler set SCR.TE and
> SCR.TEIE to 0.
>
> 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
> @@ -860,9 +860,16 @@ static void sci_transmit_chars(struct uart_port *port)
>
>         if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>                 uart_write_wakeup(port);
> -       if (uart_circ_empty(xmit))
> -               sci_stop_tx(port);
> +       if (uart_circ_empty(xmit)) {
> +               if (port->type == PORT_SCI) {
> +                       ctrl = serial_port_in(port, SCSCR);
> +                       ctrl &= ~SCSCR_TIE;
> +                       ctrl |= SCSCR_TEIE;
> +                       serial_port_out(port, SCSCR, ctrl);

Clearing SCSCR_TIE is already done in sci_stop_tx() below,
so I think it would be better to just add

    if (port->type == PORT_SCI)
            ctrl |= SCSCR_TEIE

to sci_stop_tx() instead.

> +               }
>
> +               sci_stop_tx(port);
> +       }
>  }

The rest LGTM.

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

RE: [PATCH v4 4/5] tty: serial: sh-sci: Add support for tx end interrupt handling

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Friday, April 21, 2023 10:38 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 4/5] tty: serial: sh-sci: Add support for tx end
> interrupt handling
> 
> Hi Biju,
> 
> On Wed, Apr 12, 2023 at 4:57 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > As per the RZ/G2L users hardware manual (Rev.1.20 Sep, 2022), section
> > 23.3.7 Serial Data Transmission (Asynchronous Mode), it is mentioned
> > that, set the SCR.TIE bit to 0 and SCR.TEIE bit to 1, after the last
> > data to be transmitted are written to the TDR.
> >
> > This will generate tx end interrupt and in the handler set SCR.TE and
> > SCR.TEIE to 0.
> >
> > 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
> > @@ -860,9 +860,16 @@ static void sci_transmit_chars(struct uart_port
> > *port)
> >
> >         if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> >                 uart_write_wakeup(port);
> > -       if (uart_circ_empty(xmit))
> > -               sci_stop_tx(port);
> > +       if (uart_circ_empty(xmit)) {
> > +               if (port->type == PORT_SCI) {
> > +                       ctrl = serial_port_in(port, SCSCR);
> > +                       ctrl &= ~SCSCR_TIE;
> > +                       ctrl |= SCSCR_TEIE;
> > +                       serial_port_out(port, SCSCR, ctrl);
> 
> Clearing SCSCR_TIE is already done in sci_stop_tx() below, so I think it
> would be better to just add
> 
>     if (port->type == PORT_SCI)
>             ctrl |= SCSCR_TEIE
> 
> to sci_stop_tx() instead.

I have tested this change and it works ok.

Cheers,
Biju

> 
> > +               }
> >
> > +               sci_stop_tx(port);
> > +       }
> >  }
> 
> The rest LGTM.
> 
> 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

[PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

[Biju Das]

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(+)

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;
 		}
-- 
2.25.1

Re: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

[Geert Uytterhoeven]

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

RE: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

[Biju Das]

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

Re: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

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

-- 
 i.

> >                 } 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().

RE: [PATCH v4 5/5] tty: serial: sh-sci: Fix end of transmission on SCI

[Biju Das]

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