[RFC 0/3] serial: sh-sci: SCIF FIFO exploitation

[RFC 0/3] serial: sh-sci: SCIF FIFO exploitation

[Ulrich Hecht]

Hi!

The current sh-sci implementation doesn't exploit the RX FIFOs at all and
has the chip issue an interrupt for every byte received.  This series
implements the necessary changes to improve that.

It includes a sysfs interface for changing the FIFO trigger level; there
seem to be few drivers that implement something like that (I only found
one), so I'd like to know if this implementation is palatable.

CU
Uli


Ulrich Hecht (3):
  serial: sh-sci: consider DR (data ready) bit adequately
  serial: sh-sci: exploit RX FIFOs better
  serial: sh-sci: make RX FIFO trigger tunable via sysfs

 drivers/tty/serial/sh-sci.c | 113 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/tty/serial/sh-sci.h |   5 ++
 include/linux/serial_sci.h  |   2 +
 3 files changed, 116 insertions(+), 4 deletions(-)

-- 
1.8.4.5

[RFC 1/3] serial: sh-sci: consider DR (data ready) bit adequately

[Ulrich Hecht]

To allow operation with a higher RX FIFO interrupt threshold it is
necessary to consider the DR bit ("FIFO not full, but no data received
for 1.5 frames") as an indicator that data can be read. Otherwise the
driver will let data rot in the FIFO until the threshold is reached.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 9 +++++----
 drivers/tty/serial/sh-sci.h | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 3081e46..f0ff8a7 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -483,7 +483,7 @@ static int sci_poll_get_char(struct uart_port *port)
 		break;
 	} while (1);
 
-	if (!(status & SCxSR_RDxF(port)))
+	if (!(status & (SCxSR_RDxF(port) | SCxSR_DR(port))))
 		return NO_POLL_CHAR;
 
 	c = serial_port_in(port, SCxRDR);
@@ -572,7 +572,8 @@ static int sci_rxfill(struct uart_port *port)
 	if (reg->size)
 		return serial_port_in(port, SCFDR) & ((port->fifosize << 1) - 1);
 
-	return (serial_port_in(port, SCxSR) & SCxSR_RDxF(port)) != 0;
+	return (serial_port_in(port, SCxSR) &
+	        (SCxSR_RDxF(port) | SCxSR_DR(port))) != 0;
 }
 
 /*
@@ -663,7 +664,7 @@ static void sci_receive_chars(struct uart_port *port)
 	unsigned char flag;
 
 	status = serial_port_in(port, SCxSR);
-	if (!(status & SCxSR_RDxF(port)))
+	if (!(status & (SCxSR_RDxF(port) | SCxSR_DR(port))))
 		return;
 
 	while (1) {
@@ -1014,7 +1015,7 @@ static irqreturn_t sci_mpxed_interrupt(int irq, void *ptr)
 	 * Rx Interrupt: if we're using DMA, the DMA controller clears RDF /
 	 * DR flags
 	 */
-	if (((ssr_status & SCxSR_RDxF(port)) || s->chan_rx) &&
+	if (((ssr_status & (SCxSR_RDxF(port) | SCxSR_DR(port))) || s->chan_rx) &&
 	    (scr_status & SCSCR_RIE))
 		ret = sci_rx_interrupt(irq, ptr);
 
diff --git a/drivers/tty/serial/sh-sci.h b/drivers/tty/serial/sh-sci.h
index d5db81a..9c4598d 100644
--- a/drivers/tty/serial/sh-sci.h
+++ b/drivers/tty/serial/sh-sci.h
@@ -8,6 +8,7 @@
 #define SCxSR_FER(port)		(((port)->type = PORT_SCI) ? SCI_FER    : SCIF_FER)
 #define SCxSR_PER(port)		(((port)->type = PORT_SCI) ? SCI_PER    : SCIF_PER)
 #define SCxSR_BRK(port)		(((port)->type = PORT_SCI) ? 0x00       : SCIF_BRK)
+#define SCxSR_DR(port)		(((port)->type = PORT_SCI) ? 0x00       : SCIF_DR)
 
 #define SCxSR_ERRORS(port)	(to_sci_port(port)->error_mask)
 
-- 
1.8.4.5

[RFC 2/3] serial: sh-sci: exploit RX FIFOs better

[Ulrich Hecht]

Trigger interrupts at higher FIFO fill levels to reduce load.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/tty/serial/sh-sci.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/sh-sci.h |  4 +++
 include/linux/serial_sci.h  |  2 ++
 3 files changed, 74 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index f0ff8a7..78aeec1 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -120,6 +120,7 @@ struct sci_port {
 #endif
 
 	struct notifier_block		freq_transition;
+	int				rx_trigger;
 };
 
 /* Function prototypes */
@@ -303,6 +304,8 @@ static struct plat_sci_reg sci_regmap[SCIx_NR_REGTYPES][SCIx_NR_REGS] = {
 		[SCSPTR]	= { 0x20, 16 },
 		[SCLSR]		= { 0x24, 16 },
 		[HSSRR]		= { 0x40, 16 },
+		[HSRTRGR]	= { 0x54, 16 },
+		[HSTTRGR]	= { 0x58, 16 },
 	},
 
 	/*
@@ -1854,6 +1857,62 @@ static void sci_baud_calc_hscif(unsigned int bps, unsigned long freq,
 	}
 }
 
+/* Set Rx FIFO interrupt trigger level after clamping it to a supported
+ * value.
+ */
+static void scif_set_rtrg(struct uart_port *port)
+{
+	unsigned int bits;
+	struct sci_port *sci = to_sci_port(port);
+
+	if (sci->rx_trigger < 1)
+		sci->rx_trigger = 1;
+	if (sci->rx_trigger >= port->fifosize)
+		sci->rx_trigger = port->fifosize;
+
+	/* HSCIF can be set to an arbitrary level. */
+	if (sci_getreg(port, HSRTRGR)->size) {
+		serial_port_out(port, HSRTRGR, sci->rx_trigger);
+		return;
+	}
+
+	if (port->fifosize = 16) {
+		/* SCIF */
+		if (sci->rx_trigger < 4) {
+			bits = 0;
+			sci->rx_trigger = 1;
+		} else if (sci->rx_trigger < 8) {
+			bits = SCFCR_RTRG0;
+			sci->rx_trigger = 4;
+		} else if (sci->rx_trigger < 14) {
+			bits = SCFCR_RTRG1;
+			sci->rx_trigger = 8;
+		} else {
+			bits = SCFCR_RTRG0 | SCFCR_RTRG1;
+			sci->rx_trigger = 14;
+		}
+	} else if (port->fifosize = 64 || port->fifosize = 256) {
+		/* SCIFA/B */
+		if (sci->rx_trigger < 16) {
+			bits = 0;
+			sci->rx_trigger = 1;
+		} else if (sci->rx_trigger < 32) {
+			bits = SCFCR_RTRG0;
+			sci->rx_trigger = 16;
+		} else if (sci->rx_trigger < 48) {
+			bits = SCFCR_RTRG1;
+			sci->rx_trigger = 32;
+		} else {
+			bits = SCFCR_RTRG0 | SCFCR_RTRG1;
+			sci->rx_trigger = 48;
+		}
+	} else {
+		BUG();
+	}
+
+	serial_port_out(port, SCFCR, serial_port_in(port, SCFCR) | bits);
+}
+
 static void sci_reset(struct uart_port *port)
 {
 	struct plat_sci_reg *reg;
@@ -1868,6 +1927,10 @@ static void sci_reset(struct uart_port *port)
 	reg = sci_getreg(port, SCFCR);
 	if (reg->size)
 		serial_port_out(port, SCFCR, SCFCR_RFRST | SCFCR_TFRST);
+
+	if (port->fifosize > 1) {
+		scif_set_rtrg(port);
+	}
 }
 
 static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
@@ -2180,21 +2243,25 @@ static int sci_init_single(struct platform_device *dev,
 	switch (p->type) {
 	case PORT_SCIFB:
 		port->fifosize = 256;
+		sci_port->rx_trigger = 48;
 		sci_port->overrun_bit = 9;
 		sampling_rate = 16;
 		break;
 	case PORT_HSCIF:
 		port->fifosize = 128;
 		sampling_rate = 0;
+		sci_port->rx_trigger = 64;
 		sci_port->overrun_bit = 0;
 		break;
 	case PORT_SCIFA:
 		port->fifosize = 64;
+		sci_port->rx_trigger = 32;
 		sci_port->overrun_bit = 9;
 		sampling_rate = 16;
 		break;
 	case PORT_SCIF:
 		port->fifosize = 16;
+		sci_port->rx_trigger = 8;
 		if (p->regtype = SCIx_SH7705_SCIF_REGTYPE) {
 			sci_port->overrun_bit = 9;
 			sampling_rate = 16;
@@ -2205,6 +2272,7 @@ static int sci_init_single(struct platform_device *dev,
 		break;
 	default:
 		port->fifosize = 1;
+		sci_port->rx_trigger = 1;
 		sci_port->overrun_bit = 5;
 		sampling_rate = 32;
 		break;
diff --git a/drivers/tty/serial/sh-sci.h b/drivers/tty/serial/sh-sci.h
index 9c4598d..db89635 100644
--- a/drivers/tty/serial/sh-sci.h
+++ b/drivers/tty/serial/sh-sci.h
@@ -34,6 +34,10 @@
 #define SCFCR_RFRST 0x0002
 #define SCFCR_TFRST 0x0004
 #define SCFCR_MCE   0x0008
+#define SCFCR_TTRG0 0x0010
+#define SCFCR_TTRG1 0x0020
+#define SCFCR_RTRG0 0x0040
+#define SCFCR_RTRG1 0x0080
 
 #define SCI_MAJOR		204
 #define SCI_MINOR_START		8
diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
index 6c5e3bb..95f3a04 100644
--- a/include/linux/serial_sci.h
+++ b/include/linux/serial_sci.h
@@ -100,6 +100,8 @@ enum {
 	SCRFDR,				/* Receive FIFO Data Count Register */
 	SCSPTR,				/* Serial Port Register */
 	HSSRR,				/* Sampling Rate Register */
+	HSRTRGR,			/* Receive FIFO data count trigger */
+	HSTTRGR,			/* Transmit FIFO data count trigger */
 
 	SCIx_NR_REGS,
 };
-- 
1.8.4.5

[RFC 3/3] serial: sh-sci: make RX FIFO trigger tunable via sysfs

[Ulrich Hecht]

Allows tuning of the RX FIFO interrupt threshold in case there are
problems with buffer overruns or latency.

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

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 78aeec1..a72ba5c 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1913,6 +1913,30 @@ static void scif_set_rtrg(struct uart_port *port)
 	serial_port_out(port, SCFCR, serial_port_in(port, SCFCR) | bits);
 }
 
+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);
+	int r = simple_strtol(buf, NULL, 0);
+	sci->rx_trigger = r;
+	scif_set_rtrg(port);
+	return count;
+}
+
+static DEVICE_ATTR(rx_trigger_level, 0644, rx_trigger_show, rx_trigger_store);
+
 static void sci_reset(struct uart_port *port)
 {
 	struct plat_sci_reg *reg;
@@ -2516,6 +2540,11 @@ 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_trigger_level.attr);
+	}
+
 	return 0;
 }
 
@@ -2660,6 +2689,13 @@ 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_trigger_level.attr);
+		if (ret)
+			return ret;
+	}
+
 	sp->freq_transition.notifier_call = sci_notifier;
 
 	ret = cpufreq_register_notifier(&sp->freq_transition,
-- 
1.8.4.5

Re: [RFC 0/3] serial: sh-sci: SCIF FIFO exploitation

[Peter Hurley]

[ +cc GregKH, Alan ]

Hi Uli,

On 09/12/2014 04:51 AM, Ulrich Hecht wrote:
> Hi!
> 
> The current sh-sci implementation doesn't exploit the RX FIFOs at all and
> has the chip issue an interrupt for every byte received.  This series
> implements the necessary changes to improve that.

Just a general note about the use of the words exploit and exploitation.
In common usage, 'exploit' has become synonymous with 'misuse'.

Perhaps a better choice would be 'utilize' and 'utilization'?

> It includes a sysfs interface for changing the FIFO trigger level; there
> seem to be few drivers that implement something like that (I only found
> one), so I'd like to know if this implementation is palatable.

The method to extend the sysfs interface for a specific device is in
linux-next. The original was proposed by Greg and reworked by
Yoshihiro YUNOMAE; see commit 266dcff03eed0050b6af11aaf2a61ab837d7ba3f,
'Serial: allow port drivers to have a default attribute group' and
commit aef9a7bd9b676f797dd5cefd43deb30d36b976a9,
'serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers'
for how to use the default attribute group.

OTOH, I like your sysfs attribute name, rx_trigger_level, better than
the new 8250 attribute name, rx_trig_bytes. In one sense Greg was right;
the coming explosion of different device attributes with different handling
behavior is going to suck.

Regards,
Peter Hurley

On a side note: why is there so little locking in the sh-sci driver?
Is it uniprocessor?

> CU
> Uli
> 
> 
> Ulrich Hecht (3):
>   serial: sh-sci: consider DR (data ready) bit adequately
>   serial: sh-sci: exploit RX FIFOs better
>   serial: sh-sci: make RX FIFO trigger tunable via sysfs
> 
>  drivers/tty/serial/sh-sci.c | 113 ++++++++++++++++++++++++++++++++++++++++++--
>  drivers/tty/serial/sh-sci.h |   5 ++
>  include/linux/serial_sci.h  |   2 +
>  3 files changed, 116 insertions(+), 4 deletions(-)
>