[PATCH v3 0/5] Renesas SCI fixes

[PATCH v3 0/5] Renesas SCI fixes

[Biju Das]

RZ/G2UL SMARC EVK has sci pins exposed through PMOD1 interface.
On testing, found that irq handler, tx and rx is broken.

This series fixes these issues.

v2->v3:
 * Cced stable@vger.kernel.org
 * Added Rx, Tx and Tx end interrupt handler patch.
v1->v2:
 * Replaced the wrong fixes tag
 * Added a simpler check in sci_init_single() and added a check in
   probe to catch invalid interrupt count.
Tested the SCI0 interface on RZ/G2UL by connecting to PMOD USBUART.
 39:          0     GICv3 437 Level     1004d000.serial:rx err
 40:         12     GICv3 438 Edge      1004d000.serial:rx full
 41:         70     GICv3 439 Edge      1004d000.serial:tx empty
 42:         18     GICv3 440 Level     1004d000.serial:tx end

Overlay compilation:
--------------------
Enable CONFIG_OF_OVERLAY in defconfig

cpp -nostdinc -I include -I arch -undef -x assembler-with-cpp \
  arch/arm64/boot//dts/renesas/r9a07g043u11-smarc.dts \
  r9a07g043u11-smarc.dts.tmp

dtc -@ -i include/ -I dts -O dtb -o arch/arm64/boot/dts/renesas/r9a07g043u11-smarc.dtb   r9a07g043u11-smarc.dts.tmp

cpp -nostdinc -I include -I arch -undef -x assembler-with-cpp \
  arch/arm64/boot/dts/renesas/r9a07g043-smarc.dtso \
  r9a07g043-smarc.dtso.tmp

dtc -@ -i include/ -I dts -O dtb -o arch/arm64/boot/dts/renesas/r9a07g043-smarc.dtbo   r9a07g043-smarc.dtso.tmp

Applying overlay:
-----------------
apply_overlay=tftp 0x48000000 RZ-G2UL/r9a07g043u11-smarc.dtb;fdt addr 0x48000000;fdt resize 8192;tftp 0x4A000000 RZ-G2UL/r9a07g043-smarc.dtbo;fdt apply 0x4A000000
bootcmd-overlay=run apply_overlay;tftp 0x48080000 RZ-G2UL/Image;booti 0x48080000 - 0x48000000

Biju Das (5):
  tty: serial: sh-sci: Fix transmit end interrupt handler
  tty: serial: sh-sci: Fix Rx on RZ/G2L SCI
  tty: serial: sh-sci: Fix Tx on SCI IP
  tty: serial: sh-sci: Add support for tx end interrupt handling
  arm64: dts: renesas: r9a07g044: Enable sci0 nodes using dt overlay

 arch/arm64/boot/dts/renesas/Makefile          |  1 +
 .../boot/dts/renesas/r9a07g043-smarc.dtso     | 45 ++++++++++++
 drivers/tty/serial/sh-sci.c                   | 72 +++++++++++++++++--
 drivers/tty/serial/sh-sci.h                   |  3 +
 4 files changed, 115 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g043-smarc.dtso

-- 
2.25.1

[PATCH v3 1/5] tty: serial: sh-sci: Fix transmit end interrupt handler

[Biju Das]

The fourth interrupt on SCI port is transmit end interrupt compared to
the break interrupt on other port types. So, shuffle the interrupts to fix
the transmit end interrupt handler.

Fixes: e1d0be616186 ("sh-sci: Add h8300 SCI")
Cc: stable@vger.kernel.org
Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Cced stable@vger.kernel.org
v1->v2:
 * Replaced the wrong fixes tag
 * Added a simpler check in sci_init_single() and added a check in
   probe to catch invalid interrupt count.
Tested the SCI0 interface on RZ/G2UL by connecting to PMOD USBUART.
 39:          0     GICv3 437 Level     1004d000.serial:rx err
 40:         12     GICv3 438 Edge      1004d000.serial:rx full
 41:         70     GICv3 439 Edge      1004d000.serial:tx empty
 42:         18     GICv3 440 Level     1004d000.serial:tx end
---
 drivers/tty/serial/sh-sci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index af4a7a865764..616041faab55 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -31,6 +31,7 @@
 #include <linux/ioport.h>
 #include <linux/ktime.h>
 #include <linux/major.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/of.h>
@@ -2864,6 +2865,13 @@ static int sci_init_single(struct platform_device *dev,
 			sci_port->irqs[i] = platform_get_irq(dev, i);
 	}
 
+	/*
+	 * The fourth interrupt on SCI port is transmit end interrupt, so
+	 * shuffle the interrupts.
+	 */
+	if (p->type == PORT_SCI)
+		swap(sci_port->irqs[SCIx_BRI_IRQ], sci_port->irqs[SCIx_TEI_IRQ]);
+
 	/* The SCI generates several interrupts. They can be muxed together or
 	 * connected to different interrupt lines. In the muxed case only one
 	 * interrupt resource is specified as there is only one interrupt ID.
-- 
2.25.1

[PATCH v3 2/5] tty: serial: sh-sci: Fix Rx on RZ/G2L SCI

[Biju Das]

SCI IP on RZ/G2L alike SoCs do not need regshift compared to other SCI
IPs on the SH platform. Currently, it does regshift and configuring Rx
wrongly. Drop adding regshift for RZ/G2L alike SoCs.

Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
 * New patch.
---
 drivers/tty/serial/sh-sci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 616041faab55..b9cd27451f90 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -158,6 +158,7 @@ struct sci_port {
 
 	bool has_rtscts;
 	bool autorts;
+	bool is_rz_sci;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -2937,7 +2938,7 @@ static int sci_init_single(struct platform_device *dev,
 	port->flags		= UPF_FIXED_PORT | UPF_BOOT_AUTOCONF | p->flags;
 	port->fifosize		= sci_port->params->fifosize;
 
-	if (port->type == PORT_SCI) {
+	if (port->type == PORT_SCI && !sci_port->is_rz_sci) {
 		if (sci_port->reg_size >= 0x20)
 			port->regshift = 2;
 		else
@@ -3353,6 +3354,11 @@ static int sci_probe(struct platform_device *dev)
 	sp = &sci_ports[dev_id];
 	platform_set_drvdata(dev, sp);
 
+	if (of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g043-sci") ||
+	    of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g044-sci") ||
+	    of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g054-sci"))
+		sp->is_rz_sci = 1;
+
 	ret = sci_probe_single(dev, dev_id, p, sp);
 	if (ret)
 		return ret;
-- 
2.25.1

[PATCH v3 3/5] tty: serial: sh-sci: Fix Tx on SCI IP

[Biju Das]

For SCI, the TE (transmit enable) must be set after setting TIE (transmit
interrupt enable) or in the same instruction to start the transmission.
Set TE bit in sci_start_tx() instead of set_termios() for SCI and clear
TE bit, if circular buffer is empty in sci_transmit_chars().

Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
 * New patch
---
 drivers/tty/serial/sh-sci.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index b9cd27451f90..9079a8ea9132 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -597,6 +597,15 @@ static void sci_start_tx(struct uart_port *port)
 	if (!s->chan_tx || 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);
 	}
 }
@@ -835,6 +844,12 @@ static void sci_transmit_chars(struct uart_port *port)
 			c = xmit->buf[xmit->tail];
 			xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		} else {
+			if (port->type == PORT_SCI) {
+				ctrl = serial_port_in(port, SCSCR);
+				ctrl &= ~SCSCR_TE;
+				serial_port_out(port, SCSCR, ctrl);
+				return;
+			}
 			break;
 		}
 
@@ -2581,8 +2596,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

[PATCH v3 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.

Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3:
 * New patch
---
 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 9079a8ea9132..adc2ac4a3cf6 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -862,9 +862,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)
@@ -1753,6 +1760,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;
@@ -1889,7 +1914,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..7460f6021a92 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, RZ 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 v3 2/5] tty: serial: sh-sci: Fix Rx on RZ/G2L SCI

[Geert Uytterhoeven]

Hi Biju,

On Mon, Mar 20, 2023 at 11:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> SCI IP on RZ/G2L alike SoCs do not need regshift compared to other SCI
> IPs on the SH platform. Currently, it does regshift and configuring Rx
> wrongly. Drop adding regshift for RZ/G2L alike SoCs.
>
> Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3:
>  * New patch.

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -158,6 +158,7 @@ struct sci_port {
>
>         bool has_rtscts;
>         bool autorts;
> +       bool is_rz_sci;
>  };
>
>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
> @@ -2937,7 +2938,7 @@ static int sci_init_single(struct platform_device *dev,
>         port->flags             = UPF_FIXED_PORT | UPF_BOOT_AUTOCONF | p->flags;
>         port->fifosize          = sci_port->params->fifosize;
>
> -       if (port->type == PORT_SCI) {
> +       if (port->type == PORT_SCI && !sci_port->is_rz_sci) {

Perhaps check for !dev->dev.of_node instead? Values of 1 or 2 for
regshift are only needed for sh770x/sh7750/sh7760, which don't use
DT yet.  When they are converted to DT, we can extend the driver to
support the more-or-less standard "reg-shift" DT property.

>                 if (sci_port->reg_size >= 0x20)
>                         port->regshift = 2;
>                 else
> @@ -3353,6 +3354,11 @@ static int sci_probe(struct platform_device *dev)
>         sp = &sci_ports[dev_id];
>         platform_set_drvdata(dev, sp);
>
> +       if (of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g043-sci") ||
> +           of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g044-sci") ||
> +           of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g054-sci"))

Please, no of_device_is_compatible() checks in a driver that uses
proper DT matching.

> +               sp->is_rz_sci = 1;
> +
>         ret = sci_probe_single(dev, dev_id, p, sp);
>         if (ret)
>                 return ret;

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 v3 2/5] tty: serial: sh-sci: Fix Rx on RZ/G2L SCI

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, March 20, 2023 7:12 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>;
> 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; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/5] tty: serial: sh-sci: Fix Rx on RZ/G2L SCI
> 
> Hi Biju,
> 
> On Mon, Mar 20, 2023 at 11:53 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > SCI IP on RZ/G2L alike SoCs do not need regshift compared to other SCI
> > IPs on the SH platform. Currently, it does regshift and configuring Rx
> > wrongly. Drop adding regshift for RZ/G2L alike SoCs.
> >
> > Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1]
> > nodes")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3:
> >  * New patch.
> 
> Thanks for your patch!
> 
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -158,6 +158,7 @@ struct sci_port {
> >
> >         bool has_rtscts;
> >         bool autorts;
> > +       bool is_rz_sci;
> >  };
> >
> >  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS @@ -2937,7 +2938,7
> > @@ static int sci_init_single(struct platform_device *dev,
> >         port->flags             = UPF_FIXED_PORT | UPF_BOOT_AUTOCONF | p-
> >flags;
> >         port->fifosize          = sci_port->params->fifosize;
> >
> > -       if (port->type == PORT_SCI) {
> > +       if (port->type == PORT_SCI && !sci_port->is_rz_sci) {
> 
> Perhaps check for !dev->dev.of_node instead? Values of 1 or 2 for regshift
> are only needed for sh770x/sh7750/sh7760, which don't use DT yet.  When they
> are converted to DT, we can extend the driver to support the more-or-less
> standard "reg-shift" DT property.

Agreed.

> 
> >                 if (sci_port->reg_size >= 0x20)
> >                         port->regshift = 2;
> >                 else
> > @@ -3353,6 +3354,11 @@ static int sci_probe(struct platform_device *dev)
> >         sp = &sci_ports[dev_id];
> >         platform_set_drvdata(dev, sp);
> >
> > +       if (of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g043-
> sci") ||
> > +           of_device_is_compatible(dev->dev.of_node, "renesas,r9a07g044-
> sci") ||
> > +           of_device_is_compatible(dev->dev.of_node,
> > + "renesas,r9a07g054-sci"))
> 
> Please, no of_device_is_compatible() checks in a driver that uses proper DT
> matching.

OK, will drop this.

Cheers,
Biju

> 
> > +               sp->is_rz_sci = 1;
> > +
> >         ret = sci_probe_single(dev, dev_id, p, sp);
> >         if (ret)
> >                 return ret;
> 
> 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 v3 3/5] tty: serial: sh-sci: Fix Tx on SCI IP

[Geert Uytterhoeven]

Hi Biju,

On Mon, Mar 20, 2023 at 11:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> For SCI, the TE (transmit enable) must be set after setting TIE (transmit
> interrupt enable) or in the same instruction to start the transmission.
> Set TE bit in sci_start_tx() instead of set_termios() for SCI and clear
> TE bit, if circular buffer is empty in sci_transmit_chars().
>
> Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1] nodes")

That's a DTS patch?

I'm wondering if this got broken accidentally by commit 93bcefd4c6bad4c6
("serial: sh-sci: Fix setting SCSCR_TIE while transferring data"),
which was probably never tested on SCI.

> Cc: stable@vger.kernel.org
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v3:
>  * New patch
> ---
>  drivers/tty/serial/sh-sci.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index b9cd27451f90..9079a8ea9132 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -597,6 +597,15 @@ static void sci_start_tx(struct uart_port *port)
>         if (!s->chan_tx || 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);
>         }
>  }
> @@ -835,6 +844,12 @@ static void sci_transmit_chars(struct uart_port *port)
>                         c = xmit->buf[xmit->tail];
>                         xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>                 } else {
> +                       if (port->type == PORT_SCI) {
> +                               ctrl = serial_port_in(port, SCSCR);
> +                               ctrl &= ~SCSCR_TE;
> +                               serial_port_out(port, SCSCR, ctrl);
> +                               return;
> +                       }
>                         break;
>                 }
>
> @@ -2581,8 +2596,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)) {

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 v3 3/5] tty: serial: sh-sci: Fix Tx on SCI IP

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/5] tty: serial: sh-sci: Fix Tx on SCI IP
> 
> Hi Biju,
> 
> On Mon, Mar 20, 2023 at 11:53 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > For SCI, the TE (transmit enable) must be set after setting TIE
> > (transmit interrupt enable) or in the same instruction to start the
> transmission.
> > Set TE bit in sci_start_tx() instead of set_termios() for SCI and
> > clear TE bit, if circular buffer is empty in sci_transmit_chars().
> >
> > Fixes: f9a2adcc9e90 ("arm64: dts: renesas: r9a07g044: Add SCI[0-1]
> > nodes")
> 
> That's a DTS patch?
> 
> I'm wondering if this got broken accidentally by commit 93bcefd4c6bad4c6
> ("serial: sh-sci: Fix setting SCSCR_TIE while transferring data"), which was
> probably never tested on SCI.

Looks like that patch is not tested on SCI. OK, will use the above commit
as Fixes tag.

Cheers,
Biju

> 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v3:
> >  * New patch
> > ---
> >  drivers/tty/serial/sh-sci.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index b9cd27451f90..9079a8ea9132 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -597,6 +597,15 @@ static void sci_start_tx(struct uart_port *port)
> >         if (!s->chan_tx || 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);
> >         }
> >  }
> > @@ -835,6 +844,12 @@ static void sci_transmit_chars(struct uart_port
> *port)
> >                         c = xmit->buf[xmit->tail];
> >                         xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE -
> 1);
> >                 } else {
> > +                       if (port->type == PORT_SCI) {
> > +                               ctrl = serial_port_in(port, SCSCR);
> > +                               ctrl &= ~SCSCR_TE;
> > +                               serial_port_out(port, SCSCR, ctrl);
> > +                               return;
> > +                       }
> >                         break;
> >                 }
> >
> > @@ -2581,8 +2596,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)) {
> 
> 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