[PATCH v4 0/4] Renesas *SCIF* RX FIFO support

[PATCH v4 0/4] Renesas *SCIF* RX FIFO support

[Ulrich Hecht]

Hi!

This series implements support for using RX FIFO thresholds higher than one
in PIO mode on SCIF, HSCIF, SCIFA and SCIFB serial ports.

This revision is the remainder of the series rebased to apply cleanly to
tty-testing, with some minor style adjustments as suggested by Sergei.

CU
Uli


Changes since v3:
- rebased onto tty-testing
- fixed a few style issues

Changes since v2:
- rebase
- add missing changelog entry

Changes since v1:
- clarify HS trigger register enum
- simplify DR bit handling
- if() cascade -> switch()
- disable RX trigger for SH77xx-style ports
- clean up on failure to create sysfs attribute
- r8a7796 DT: add control pins, rtscts flag


Ulrich Hecht (4):
  serial: sh-sci: increase RX FIFO trigger defaults for (H)SCIF
  serial: sh-sci: SCIFA/B RX FIFO software timeout
  serial: sh-sci: make RX FIFO parameters tunable via sysfs
  arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  14 ++
 drivers/tty/serial/sh-sci.c                        | 213 +++++++++++++++++----
 2 files changed, 195 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] serial: sh-sci: increase RX FIFO trigger defaults for (H)SCIF

[Ulrich Hecht]

Sets reasonable trigger defaults for the various SCIF variants.
Also corrects the FIFO size for SH7705-style ports.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 050d028..520e344 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -148,6 +148,7 @@ struct sci_port {
 	struct timer_list		rx_timer;
 	unsigned int			rx_timeout;
 #endif
+	int				rx_trigger;
 
 	bool has_rtscts;
 	bool autorts;
@@ -450,7 +451,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
 			[SCFCR]		= { 0x18, 16 },
 			[SCFDR]		= { 0x1c, 16 },
 		},
-		.fifosize = 16,
+		.fifosize = 64,
 		.overrun_reg = SCxSR,
 		.overrun_mask = SCIFA_ORER,
 		.sampling_rate_mask = SCI_SR(16),
@@ -2062,6 +2063,7 @@ static void sci_reset(struct uart_port *port)
 {
 	const struct plat_sci_reg *reg;
 	unsigned int status;
+	struct sci_port *s = to_sci_port(port);
 
 	do {
 		status = serial_port_in(port, SCxSR);
@@ -2081,6 +2083,9 @@ static void sci_reset(struct uart_port *port)
 		status &= ~(SCLSR_TO | SCLSR_ORER);
 		serial_port_out(port, SCLSR, status);
 	}
+
+	if (s->rx_trigger > 1)
+		scif_set_rtrg(port, s->rx_trigger);
 }
 
 static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
@@ -2615,6 +2620,28 @@ static int sci_init_single(struct platform_device *dev,
 	if (unlikely(sci_port->params == NULL))
 		return -EINVAL;
 
+	switch (p->type) {
+	case PORT_SCIFB:
+		sci_port->rx_trigger = 48;
+		break;
+	case PORT_HSCIF:
+		sci_port->rx_trigger = 64;
+		break;
+	case PORT_SCIFA:
+		sci_port->rx_trigger = 32;
+		break;
+	case PORT_SCIF:
+		if (p->regtype == SCIx_SH7705_SCIF_REGTYPE)
+			/* RX triggering not implemented for this IP */
+			sci_port->rx_trigger = 1;
+		else
+			sci_port->rx_trigger = 8;
+		break;
+	default:
+		sci_port->rx_trigger = 1;
+		break;
+	}
+
 	/* SCIFA on sh7723 and sh7724 need a custom sampling rate that doesn't
 	 * match the SoC datasheet, this should be investigated. Let platform
 	 * data override the sampling rate for now.
-- 
2.7.4

Re: [PATCH v4 1/4] serial: sh-sci: increase RX FIFO trigger defaults for (H)SCIF

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Sets reasonable trigger defaults for the various SCIF variants.
> Also corrects the FIFO size for SH7705-style ports.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

This patch (which is now in tty-next) breaks serial console input on
systems with a SCIFA serial port (r8a73a4/ape6evm, r8a7740/armadillo,
and sh73a0/kzm9g).  Console input isn't received by the system, until
more characters have been accumulated in the FIFO than the trigger value
(32 on SCIFA), or until serial output is generated.

Unfortunately the later patches in this series do not fix this (except
by doing "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" manually).

SCIF serial consoles (on R-Car Gen2 and Gen3) seem to keep on working
fine.

> ---
>  drivers/tty/serial/sh-sci.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 050d028..520e344 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -148,6 +148,7 @@ struct sci_port {
>         struct timer_list               rx_timer;
>         unsigned int                    rx_timeout;
>  #endif
> +       int                             rx_trigger;
>
>         bool has_rtscts;
>         bool autorts;
> @@ -450,7 +451,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>                         [SCFCR]         = { 0x18, 16 },
>                         [SCFDR]         = { 0x1c, 16 },
>                 },
> -               .fifosize = 16,
> +               .fifosize = 64,
>                 .overrun_reg = SCxSR,
>                 .overrun_mask = SCIFA_ORER,
>                 .sampling_rate_mask = SCI_SR(16),
> @@ -2062,6 +2063,7 @@ static void sci_reset(struct uart_port *port)
>  {
>         const struct plat_sci_reg *reg;
>         unsigned int status;
> +       struct sci_port *s = to_sci_port(port);
>
>         do {
>                 status = serial_port_in(port, SCxSR);
> @@ -2081,6 +2083,9 @@ static void sci_reset(struct uart_port *port)
>                 status &= ~(SCLSR_TO | SCLSR_ORER);
>                 serial_port_out(port, SCLSR, status);
>         }
> +
> +       if (s->rx_trigger > 1)
> +               scif_set_rtrg(port, s->rx_trigger);
>  }
>
>  static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
> @@ -2615,6 +2620,28 @@ static int sci_init_single(struct platform_device *dev,
>         if (unlikely(sci_port->params == NULL))
>                 return -EINVAL;
>
> +       switch (p->type) {
> +       case PORT_SCIFB:
> +               sci_port->rx_trigger = 48;
> +               break;
> +       case PORT_HSCIF:
> +               sci_port->rx_trigger = 64;
> +               break;
> +       case PORT_SCIFA:
> +               sci_port->rx_trigger = 32;
> +               break;
> +       case PORT_SCIF:
> +               if (p->regtype == SCIx_SH7705_SCIF_REGTYPE)
> +                       /* RX triggering not implemented for this IP */
> +                       sci_port->rx_trigger = 1;
> +               else
> +                       sci_port->rx_trigger = 8;
> +               break;
> +       default:
> +               sci_port->rx_trigger = 1;
> +               break;
> +       }
> +
>         /* SCIFA on sh7723 and sh7724 need a custom sampling rate that doesn't
>          * match the SoC datasheet, this should be investigated. Let platform
>          * data override the sampling rate for now.
> --
> 2.7.4

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/4] serial: sh-sci: SCIFA/B RX FIFO software timeout

[Ulrich Hecht]

Implements support for FIFO fill thresholds greater than one with software
timeout.

This mechanism is not possible (or at least not useful) on SCIF family
hardware other than SCIFA and SCIFB because they do not support turning off
the DR hardware timeout interrupt separately from the RI interrupt.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 101 +++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 33 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 520e344..4a165ed 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -148,7 +148,10 @@ struct sci_port {
 	struct timer_list		rx_timer;
 	unsigned int			rx_timeout;
 #endif
+	unsigned int			rx_frame;
 	int				rx_trigger;
+	struct timer_list		rx_fifo_timer;
+	int				rx_fifo_timeout;
 
 	bool has_rtscts;
 	bool autorts;
@@ -1034,6 +1037,24 @@ static int scif_set_rtrg(struct uart_port *port, int rx_trig)
 	return rx_trig;
 }
 
+static int scif_rtrg_enabled(struct uart_port *port)
+{
+	if (sci_getreg(port, HSRTRGR)->size)
+		return serial_port_in(port, HSRTRGR) != 0;
+	else
+		return (serial_port_in(port, SCFCR) &
+			(SCFCR_RTRG0 | SCFCR_RTRG1)) != 0;
+}
+
+static void rx_fifo_timer_fn(unsigned long arg)
+{
+	struct sci_port *s = (struct sci_port *)arg;
+	struct uart_port *port = &s->port;
+
+	dev_dbg(port->dev, "Rx timed out\n");
+	scif_set_rtrg(port, 1);
+}
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 static void sci_dma_tx_complete(void *arg)
 {
@@ -1473,10 +1494,10 @@ static inline void sci_free_dma(struct uart_port *port)
 
 static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 {
-#ifdef CONFIG_SERIAL_SH_SCI_DMA
 	struct uart_port *port = ptr;
 	struct sci_port *s = to_sci_port(port);
 
+#ifdef CONFIG_SERIAL_SH_SCI_DMA
 	if (s->chan_rx) {
 		u16 scr = serial_port_in(port, SCSCR);
 		u16 ssr = serial_port_in(port, SCxSR);
@@ -1501,6 +1522,14 @@ static irqreturn_t sci_rx_interrupt(int irq, void *ptr)
 	}
 #endif
 
+	if (s->rx_trigger > 1 && s->rx_fifo_timeout > 0) {
+		if (!scif_rtrg_enabled(port))
+			scif_set_rtrg(port, s->rx_trigger);
+
+		mod_timer(&s->rx_fifo_timer, jiffies + DIV_ROUND_UP(
+			  s->rx_frame * s->rx_fifo_timeout, 1000));
+	}
+
 	/* I think sci_receive_chars has to be called irrespective
 	 * of whether the I_IXOFF is set, otherwise, how is the interrupt
 	 * to be disabled?
@@ -2084,14 +2113,21 @@ static void sci_reset(struct uart_port *port)
 		serial_port_out(port, SCLSR, status);
 	}
 
-	if (s->rx_trigger > 1)
-		scif_set_rtrg(port, s->rx_trigger);
+	if (s->rx_trigger > 1) {
+		if (s->rx_fifo_timeout) {
+			scif_set_rtrg(port, 1);
+			setup_timer(&s->rx_fifo_timer, rx_fifo_timer_fn,
+				    (unsigned long)s);
+		} else {
+			scif_set_rtrg(port, s->rx_trigger);
+		}
+	}
 }
 
 static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
 			    struct ktermios *old)
 {
-	unsigned int baud, smr_val = SCSMR_ASYNC, scr_val = 0, i;
+	unsigned int baud, smr_val = SCSMR_ASYNC, scr_val = 0, i, bits;
 	unsigned int brr = 255, cks = 0, srr = 15, dl = 0, sccks = 0;
 	unsigned int brr1 = 255, cks1 = 0, srr1 = 15, dl1 = 0;
 	struct sci_port *s = to_sci_port(port);
@@ -2287,7 +2323,6 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
 		udelay(DIV_ROUND_UP(10 * 1000000, baud));
 	}
 
-#ifdef CONFIG_SERIAL_SH_SCI_DMA
 	/*
 	 * Calculate delay for 2 DMA buffers (4 FIFO).
 	 * See serial_core.c::uart_update_timeout().
@@ -2298,36 +2333,34 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * value obtained by this formula is too small. Therefore, if the value
 	 * is smaller than 20ms, use 20ms as the timeout value for DMA.
 	 */
-	if (s->chan_rx) {
-		unsigned int bits;
+	/* byte size and parity */
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		bits = 7;
+		break;
+	case CS6:
+		bits = 8;
+		break;
+	case CS7:
+		bits = 9;
+		break;
+	default:
+		bits = 10;
+		break;
+	}
 
-		/* byte size and parity */
-		switch (termios->c_cflag & CSIZE) {
-		case CS5:
-			bits = 7;
-			break;
-		case CS6:
-			bits = 8;
-			break;
-		case CS7:
-			bits = 9;
-			break;
-		default:
-			bits = 10;
-			break;
-		}
+	if (termios->c_cflag & CSTOPB)
+		bits++;
+	if (termios->c_cflag & PARENB)
+		bits++;
 
-		if (termios->c_cflag & CSTOPB)
-			bits++;
-		if (termios->c_cflag & PARENB)
-			bits++;
-		s->rx_timeout = DIV_ROUND_UP((s->buf_len_rx * 2 * bits * HZ) /
-					     (baud / 10), 10);
-		dev_dbg(port->dev, "DMA Rx t-out %ums, tty t-out %u jiffies\n",
-			s->rx_timeout * 1000 / HZ, port->timeout);
-		if (s->rx_timeout < msecs_to_jiffies(20))
-			s->rx_timeout = msecs_to_jiffies(20);
-	}
+	s->rx_frame = (100 * bits * HZ) / (baud / 10);
+#ifdef CONFIG_SERIAL_SH_SCI_DMA
+	s->rx_timeout = DIV_ROUND_UP(s->buf_len_rx * 2 * s->rx_frame, 1000);
+	dev_dbg(port->dev, "DMA Rx t-out %ums, tty t-out %u jiffies\n",
+		s->rx_timeout * 1000 / HZ, port->timeout);
+	if (s->rx_timeout < msecs_to_jiffies(20))
+		s->rx_timeout = msecs_to_jiffies(20);
 #endif
 
 	if ((termios->c_cflag & CREAD) != 0)
@@ -2642,6 +2675,8 @@ static int sci_init_single(struct platform_device *dev,
 		break;
 	}
 
+	sci_port->rx_fifo_timeout = 0;
+
 	/* SCIFA on sh7723 and sh7724 need a custom sampling rate that doesn't
 	 * match the SoC datasheet, this should be investigated. Let platform
 	 * data override the sampling rate for now.
-- 
2.7.4

[PATCH v4 3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Ulrich Hecht]

Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
only applicable to SCIFA and SCIFB).

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 4a165ed..f95a56c 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
 	scif_set_rtrg(port, 1);
 }
 
+static ssize_t rx_trigger_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct sci_port *sci = to_sci_port(port);
+
+	return sprintf(buf, "%d\n", sci->rx_trigger);
+}
+
+static ssize_t rx_trigger_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t count)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct sci_port *sci = to_sci_port(port);
+	long r;
+
+	if (kstrtol(buf, 0, &r) == -EINVAL)
+		return -EINVAL;
+	sci->rx_trigger = scif_set_rtrg(port, r);
+	scif_set_rtrg(port, 1);
+	return count;
+}
+
+static DEVICE_ATTR(rx_fifo_trigger, 0644, rx_trigger_show, rx_trigger_store);
+
+static ssize_t rx_fifo_timeout_show(struct device *dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct sci_port *sci = to_sci_port(port);
+
+	return sprintf(buf, "%d\n", sci->rx_fifo_timeout);
+}
+
+static ssize_t rx_fifo_timeout_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t count)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct sci_port *sci = to_sci_port(port);
+	long r;
+
+	if (kstrtol(buf, 0, &r) == -EINVAL)
+		return -EINVAL;
+	sci->rx_fifo_timeout = r;
+	scif_set_rtrg(port, 1);
+	if (r > 0)
+		setup_timer(&sci->rx_fifo_timer, rx_fifo_timer_fn,
+			    (unsigned long)sci);
+	return count;
+}
+
+static DEVICE_ATTR(rx_fifo_timeout, 0644, rx_fifo_timeout_show, rx_fifo_timeout_store);
+
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 static void sci_dma_tx_complete(void *arg)
 {
@@ -2886,6 +2946,15 @@ static int sci_remove(struct platform_device *dev)
 
 	sci_cleanup_single(port);
 
+	if (port->port.fifosize > 1) {
+		sysfs_remove_file(&dev->dev.kobj,
+				  &dev_attr_rx_fifo_trigger.attr);
+	}
+	if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB) {
+		sysfs_remove_file(&dev->dev.kobj,
+				  &dev_attr_rx_fifo_timeout.attr);
+	}
+
 	return 0;
 }
 
@@ -3051,6 +3120,24 @@ static int sci_probe(struct platform_device *dev)
 	if (ret)
 		return ret;
 
+	if (sp->port.fifosize > 1) {
+		ret = sysfs_create_file(&dev->dev.kobj,
+				&dev_attr_rx_fifo_trigger.attr);
+		if (ret)
+			return ret;
+	}
+	if (sp->port.type == PORT_SCIFA || sp->port.type ==  PORT_SCIFB) {
+		ret = sysfs_create_file(&dev->dev.kobj,
+				&dev_attr_rx_fifo_timeout.attr);
+		if (ret) {
+			if (sp->port.fifosize > 1) {
+				sysfs_remove_file(&dev->dev.kobj,
+					&dev_attr_rx_fifo_trigger.attr);
+			}
+			return ret;
+		}
+	}
+
 #ifdef CONFIG_SH_STANDARD_BIOS
 	sh_bios_gdb_detach();
 #endif
-- 
2.7.4

Re: [PATCH v4 3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
> only applicable to SCIFA and SCIFB).
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 4a165ed..f95a56c 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>         scif_set_rtrg(port, 1);
>  }
>
> +static ssize_t rx_trigger_show(struct device *dev,
> +                              struct device_attribute *attr,
> +                              char *buf)
> +{
> +       struct uart_port *port = dev_get_drvdata(dev);
> +       struct sci_port *sci = to_sci_port(port);
> +
> +       return sprintf(buf, "%d\n", sci->rx_trigger);
> +}
> +
> +static ssize_t rx_trigger_store(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf,
> +                               size_t count)
> +{
> +       struct uart_port *port = dev_get_drvdata(dev);
> +       struct sci_port *sci = to_sci_port(port);
> +       long r;
> +
> +       if (kstrtol(buf, 0, &r) == -EINVAL)
> +               return -EINVAL;
> +       sci->rx_trigger = scif_set_rtrg(port, r);
> +       scif_set_rtrg(port, 1);

I seem to have missed the above function call during my earlier review.
What's the purpose of resetting the trigger immediately to 1?

This means I can change the value of scif->rx_trigger on the fly, but the
actual value programmed in the hardware is always 1?

I.e. "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" fixes serial
console input on e.g. armadillo, but echoing 32 into rx_fifo_trigger doesn't
break it again.

> +       return count;
> +}

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 3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Ulrich Hecht]

On Tue, Feb 7, 2017 at 3:40 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulrich,
>
> On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
> <ulrich.hecht+renesas@gmail.com> wrote:
>> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
>> only applicable to SCIFA and SCIFB).
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 87 insertions(+)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index 4a165ed..f95a56c 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>>         scif_set_rtrg(port, 1);
>>  }
>>
>> +static ssize_t rx_trigger_show(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              char *buf)
>> +{
>> +       struct uart_port *port = dev_get_drvdata(dev);
>> +       struct sci_port *sci = to_sci_port(port);
>> +
>> +       return sprintf(buf, "%d\n", sci->rx_trigger);
>> +}
>> +
>> +static ssize_t rx_trigger_store(struct device *dev,
>> +                               struct device_attribute *attr,
>> +                               const char *buf,
>> +                               size_t count)
>> +{
>> +       struct uart_port *port = dev_get_drvdata(dev);
>> +       struct sci_port *sci = to_sci_port(port);
>> +       long r;
>> +
>> +       if (kstrtol(buf, 0, &r) == -EINVAL)
>> +               return -EINVAL;
>> +       sci->rx_trigger = scif_set_rtrg(port, r);
>> +       scif_set_rtrg(port, 1);
>
> I seem to have missed the above function call during my earlier review.
> What's the purpose of resetting the trigger immediately to 1?

For the software timeout case, the timeout and the trigger levels are
set in the interrupt handler. Setting the threshold to 1 will trigger
that when the next byte of data comes in, and it is easier than
duplicating the logic here.

(There actually is a bug here, in that the threshold should only be
reset to 1 for software timeout IPs (SCIFA and SCIFB), but that is not
what breaks SCIFA, of course.)

> I.e. "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" fixes serial
> console input on e.g. armadillo, but echoing 32 into rx_fifo_trigger doesn't
> break it again.

This is intended to work that way. For software timeout devices (SCIFA
and SCIFB), the trigger level is not set in hardware unless an
rx_fifo_timeout > 0 is set as well.

The bug that breaks the SCIFA console is in sci_reset(), which sets a
hardware threshold > 1 for devices for software timeout devices even
though the rx_fifo_timeout is 0. Something like this should fix it:

--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2179,7 +2179,11 @@ static void sci_reset(struct uart_port *port)
                        setup_timer(&s->rx_fifo_timer, rx_fifo_timer_fn,
                                    (unsigned long)s);
                } else {
-                       scif_set_rtrg(port, s->rx_trigger);
+                       if (port->type == PORT_SCIFA ||
+                           port->type == PORT_SCIFB)
+                               scif_set_rtrg(port, 1);
+                       else
+                               scif_set_rtrg(port, s->rx_trigger);
                }
        }
 }

Could you try and check if that works for you?

CU
Uli

Re: [PATCH v4 3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Geert Uytterhoeven]

Hi Uli,

On Wed, Feb 8, 2017 at 11:04 AM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> On Tue, Feb 7, 2017 at 3:40 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Feb 3, 2017 at 11:38 AM, Ulrich Hecht
>> <ulrich.hecht+renesas@gmail.com> wrote:
>>> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
>>> only applicable to SCIFA and SCIFB).
>>>
>>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/tty/serial/sh-sci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 87 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>>> index 4a165ed..f95a56c 100644
>>> --- a/drivers/tty/serial/sh-sci.c
>>> +++ b/drivers/tty/serial/sh-sci.c
>>> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>>>         scif_set_rtrg(port, 1);
>>>  }
>>>
>>> +static ssize_t rx_trigger_show(struct device *dev,
>>> +                              struct device_attribute *attr,
>>> +                              char *buf)
>>> +{
>>> +       struct uart_port *port = dev_get_drvdata(dev);
>>> +       struct sci_port *sci = to_sci_port(port);
>>> +
>>> +       return sprintf(buf, "%d\n", sci->rx_trigger);
>>> +}
>>> +
>>> +static ssize_t rx_trigger_store(struct device *dev,
>>> +                               struct device_attribute *attr,
>>> +                               const char *buf,
>>> +                               size_t count)
>>> +{
>>> +       struct uart_port *port = dev_get_drvdata(dev);
>>> +       struct sci_port *sci = to_sci_port(port);
>>> +       long r;
>>> +
>>> +       if (kstrtol(buf, 0, &r) == -EINVAL)
>>> +               return -EINVAL;
>>> +       sci->rx_trigger = scif_set_rtrg(port, r);
>>> +       scif_set_rtrg(port, 1);
>>
>> I seem to have missed the above function call during my earlier review.
>> What's the purpose of resetting the trigger immediately to 1?
>
> For the software timeout case, the timeout and the trigger levels are
> set in the interrupt handler. Setting the threshold to 1 will trigger
> that when the next byte of data comes in, and it is easier than
> duplicating the logic here.

OK.

> (There actually is a bug here, in that the threshold should only be
> reset to 1 for software timeout IPs (SCIFA and SCIFB), but that is not
> what breaks SCIFA, of course.)

I guess you'll send a patch to fix that, too?

>> I.e. "echo 1 > /sys/class/tty/ttySC0/device/rx_fifo_trigger" fixes serial
>> console input on e.g. armadillo, but echoing 32 into rx_fifo_trigger doesn't
>> break it again.
>
> This is intended to work that way. For software timeout devices (SCIFA
> and SCIFB), the trigger level is not set in hardware unless an
> rx_fifo_timeout > 0 is set as well.
>
> The bug that breaks the SCIFA console is in sci_reset(), which sets a
> hardware threshold > 1 for devices for software timeout devices even
> though the rx_fifo_timeout is 0. Something like this should fix it:
>
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2179,7 +2179,11 @@ static void sci_reset(struct uart_port *port)
>                         setup_timer(&s->rx_fifo_timer, rx_fifo_timer_fn,
>                                     (unsigned long)s);
>                 } else {
> -                       scif_set_rtrg(port, s->rx_trigger);
> +                       if (port->type == PORT_SCIFA ||
> +                           port->type == PORT_SCIFB)
> +                               scif_set_rtrg(port, 1);
> +                       else
> +                               scif_set_rtrg(port, s->rx_trigger);
>                 }
>         }
>  }
>
> Could you try and check if that works for you?

Thanks, that unbroke serial console input on all my boards with SCIFA consoles.

Tested-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 3/4] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Friday 03 Feb 2017 11:38:19 Ulrich Hecht wrote:
> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
> only applicable to SCIFA and SCIFB).
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/sh-sci.c | 87 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 4a165ed..f95a56c 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1055,6 +1055,66 @@ static void rx_fifo_timer_fn(unsigned long arg)
>  	scif_set_rtrg(port, 1);
>  }
> 
> +static ssize_t rx_trigger_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct sci_port *sci = to_sci_port(port);
> +
> +	return sprintf(buf, "%d\n", sci->rx_trigger);
> +}
> +
> +static ssize_t rx_trigger_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t count)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct sci_port *sci = to_sci_port(port);
> +	long r;
> +
> +	if (kstrtol(buf, 0, &r) == -EINVAL)
> +		return -EINVAL;
> +	sci->rx_trigger = scif_set_rtrg(port, r);
> +	scif_set_rtrg(port, 1);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(rx_fifo_trigger, 0644, rx_trigger_show,
> rx_trigger_store); +
> +static ssize_t rx_fifo_timeout_show(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct sci_port *sci = to_sci_port(port);
> +
> +	return sprintf(buf, "%d\n", sci->rx_fifo_timeout);
> +}
> +
> +static ssize_t rx_fifo_timeout_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t count)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct sci_port *sci = to_sci_port(port);
> +	long r;
> +
> +	if (kstrtol(buf, 0, &r) == -EINVAL)
> +		return -EINVAL;
> +	sci->rx_fifo_timeout = r;
> +	scif_set_rtrg(port, 1);
> +	if (r > 0)
> +		setup_timer(&sci->rx_fifo_timer, rx_fifo_timer_fn,
> +			    (unsigned long)sci);

Where's the locking ?

> +	return count;
> +}
> +
> +static DEVICE_ATTR(rx_fifo_timeout, 0644, rx_fifo_timeout_show,
> rx_fifo_timeout_store); +
> +
>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>  static void sci_dma_tx_complete(void *arg)
>  {
> @@ -2886,6 +2946,15 @@ static int sci_remove(struct platform_device *dev)
> 
>  	sci_cleanup_single(port);
> 
> +	if (port->port.fifosize > 1) {
> +		sysfs_remove_file(&dev->dev.kobj,
> +				  &dev_attr_rx_fifo_trigger.attr);
> +	}
> +	if (port->port.type == PORT_SCIFA || port->port.type == PORT_SCIFB) {
> +		sysfs_remove_file(&dev->dev.kobj,
> +				  &dev_attr_rx_fifo_timeout.attr);
> +	}

No need for curly braces.

> +
>  	return 0;
>  }
> 
> @@ -3051,6 +3120,24 @@ static int sci_probe(struct platform_device *dev)
>  	if (ret)
>  		return ret;
> 
> +	if (sp->port.fifosize > 1) {
> +		ret = sysfs_create_file(&dev->dev.kobj,
> +				&dev_attr_rx_fifo_trigger.attr);

You should use device_create_file for device attributes. Even better would be 
to create an attribute group if possible.

> +		if (ret)
> +			return ret;
> +	}
> +	if (sp->port.type == PORT_SCIFA || sp->port.type ==  PORT_SCIFB) {
> +		ret = sysfs_create_file(&dev->dev.kobj,
> +				&dev_attr_rx_fifo_timeout.attr);
> +		if (ret) {
> +			if (sp->port.fifosize > 1) {
> +				sysfs_remove_file(&dev->dev.kobj,
> +					&dev_attr_rx_fifo_trigger.attr);
> +			}
> +			return ret;
> +		}
> +	}
> +
>  #ifdef CONFIG_SH_STANDARD_BIOS
>  	sh_bios_gdb_detach();
>  #endif

-- 
Regards,

Laurent Pinchart

[PATCH v4 4/4] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Ulrich Hecht]

Enables the SCIF hooked up to the DEBUG1 connector.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
index f35e96c..443eb0a 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
@@ -18,6 +18,7 @@
 
 	aliases {
 		serial0 = &scif2;
+		serial1 = &scif1;
 	};
 
 	chosen {
@@ -102,6 +103,11 @@
 	pinctrl-0 = <&scif_clk_pins>;
 	pinctrl-names = "default";
 
+	scif1_pins: scif1 {
+		groups = "scif1_data_a", "scif1_ctrl";
+		function = "scif1";
+	};
+
 	scif2_pins: scif2 {
 		groups = "scif2_data_a";
 		function = "scif2";
@@ -202,6 +208,14 @@
 	status = "okay";
 };
 
+&scif1 {
+	pinctrl-0 = <&scif1_pins>;
+	pinctrl-names = "default";
+
+	uart-has-rtscts;
+	status = "okay";
+};
+
 &scif2 {
 	pinctrl-0 = <&scif2_pins>;
 	pinctrl-names = "default";
-- 
2.7.4

Re: [PATCH v4 4/4] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Simon Horman]

On Fri, Feb 03, 2017 at 11:38:20AM +0100, Ulrich Hecht wrote:
> Enables the SCIF hooked up to the DEBUG1 connector.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Hi Ulrich,

could you clarify the dependency of this patch on earlier ones in the
series. Is it safe to queue this up independently of the other patches?

Re: [PATCH v4 4/4] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Ulrich Hecht]

On Wed, Feb 8, 2017 at 11:54 AM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Feb 03, 2017 at 11:38:20AM +0100, Ulrich Hecht wrote:
>> Enables the SCIF hooked up to the DEBUG1 connector.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Hi Ulrich,
>
> could you clarify the dependency of this patch on earlier ones in the
> series. Is it safe to queue this up independently of the other patches?

It does not actually depend on the other patches. I think I included
it here because I used that port for testing. Should be safe.

CU
Uli

Re: [PATCH v4 4/4] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Simon Horman]

On Wed, Feb 08, 2017 at 12:19:55PM +0100, Ulrich Hecht wrote:
> On Wed, Feb 8, 2017 at 11:54 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Feb 03, 2017 at 11:38:20AM +0100, Ulrich Hecht wrote:
> >> Enables the SCIF hooked up to the DEBUG1 connector.
> >>
> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Hi Ulrich,
> >
> > could you clarify the dependency of this patch on earlier ones in the
> > series. Is it safe to queue this up independently of the other patches?
> 
> It does not actually depend on the other patches. I think I included
> it here because I used that port for testing. Should be safe.

Thanks but I dont' seem to have scif1 in r8a7796.dtsi.
Should this patch be updated to use hscif1 which is (now) present in
r8a7796.dtsi?

Re: [PATCH v4 4/4] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Geert Uytterhoeven]

Hi Simon,

On Mon, Feb 13, 2017 at 2:05 PM, Simon Horman <horms@verge.net.au> wrote:
> On Wed, Feb 08, 2017 at 12:19:55PM +0100, Ulrich Hecht wrote:
>> On Wed, Feb 8, 2017 at 11:54 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Fri, Feb 03, 2017 at 11:38:20AM +0100, Ulrich Hecht wrote:
>> >> Enables the SCIF hooked up to the DEBUG1 connector.
>> >>
>> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> >
>> > Hi Ulrich,
>> >
>> > could you clarify the dependency of this patch on earlier ones in the
>> > series. Is it safe to queue this up independently of the other patches?
>>
>> It does not actually depend on the other patches. I think I included
>> it here because I used that port for testing. Should be safe.
>
> Thanks but I dont' seem to have scif1 in r8a7796.dtsi.
> Should this patch be updated to use hscif1 which is (now) present in
> r8a7796.dtsi?

Nah, you just need

[PATCH v2 2/3] arm64: renesas: r8a7796: Add all SCIF nodes

first :-)

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