[PATCH 0/6] sc16is7xx: Hardware flow control fixes

[PATCH 0/6] sc16is7xx: Hardware flow control fixes

[Tomasz Moń]

sc16is7xx driver assumes that the device handles hardware flow control
automatically. This is not really true as the driver does inadvertently
clear the bits that enable hardware flow control.

This patch series solves multiple issues present in the driver. While
the patches are fairly independent, there are some dependencies. The
"sc16is7xx: Properly resume TX after stop" adds IER bit set function
that is later used in "sc16is7xx: Set AUTOCTS and AUTORTS bits". Also
the patches that control which interrupts are enabled are dependent on
each other.

Patches should be applied respecting the order in the series. The whole
series applies on top of "sc16is7xx: Fix for incorrect data being
transmitted" [1].

Patch series has been developed and tested on a board with SC16IS760
connected via SPI bus.

[1] https://lore.kernel.org/linux-serial/20220216160802.1026013-1-phil@raspberrypi.com/

Lech Perczak (3):
  sc16is7xx: Preserve EFR bits on update
  sc16is7xx: Update status lines in single call
  sc16is7xx: Separate GPIOs from modem control lines

Tomasz Moń (3):
  sc16is7xx: Properly resume TX after stop
  sc16is7xx: Handle modem status lines
  sc16is7xx: Set AUTOCTS and AUTORTS bits

 drivers/tty/serial/sc16is7xx.c | 257 +++++++++++++++++++++++++++++----
 1 file changed, 227 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/6] sc16is7xx: Preserve EFR bits on update

[Tomasz Moń]

From: Lech Perczak <l.perczak@camlintechnologies.com>

Preserve unaffected bits state when accessing EFR register. This
prevents hardware flow control bits from being cleared on enhanced
functions access.

Signed-off-by: Lech Perczak <l.perczak@camlintechnologies.com>
Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 38d1c0748533..3800733452fe 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -289,6 +289,14 @@
 						  *       XON1, XON2, XOFF1 and
 						  *       XOFF2
 						  */
+#define SC16IS7XX_EFR_FLOWCTRL_BITS	(SC16IS7XX_EFR_AUTORTS_BIT | \
+					SC16IS7XX_EFR_AUTOCTS_BIT | \
+					SC16IS7XX_EFR_XOFF2_DETECT_BIT | \
+					SC16IS7XX_EFR_SWFLOW3_BIT | \
+					SC16IS7XX_EFR_SWFLOW2_BIT | \
+					SC16IS7XX_EFR_SWFLOW1_BIT | \
+					SC16IS7XX_EFR_SWFLOW0_BIT)
+
 
 /* Misc definitions */
 #define SC16IS7XX_FIFO_SIZE		(64)
@@ -523,8 +531,10 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 
 	/* Enable enhanced features */
 	regcache_cache_bypass(s->regmap, true);
-	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
-			     SC16IS7XX_EFR_ENABLE_BIT);
+	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
+			      SC16IS7XX_EFR_ENABLE_BIT,
+			      SC16IS7XX_EFR_ENABLE_BIT);
+
 	regcache_cache_bypass(s->regmap, false);
 
 	/* Put LCR back to the normal mode */
@@ -935,7 +945,10 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	if (termios->c_iflag & IXOFF)
 		flow |= SC16IS7XX_EFR_SWFLOW1_BIT;
 
-	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG, flow);
+	sc16is7xx_port_update(port,
+			      SC16IS7XX_EFR_REG,
+			      SC16IS7XX_EFR_FLOWCTRL_BITS,
+			      flow);
 	regcache_cache_bypass(s->regmap, false);
 
 	/* Update LCR register */
@@ -1010,8 +1023,9 @@ static int sc16is7xx_startup(struct uart_port *port)
 	regcache_cache_bypass(s->regmap, true);
 
 	/* Enable write access to enhanced features and internal clock div */
-	sc16is7xx_port_write(port, SC16IS7XX_EFR_REG,
-			     SC16IS7XX_EFR_ENABLE_BIT);
+	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
+			      SC16IS7XX_EFR_ENABLE_BIT,
+			      SC16IS7XX_EFR_ENABLE_BIT);
 
 	/* Enable TCR/TLR */
 	sc16is7xx_port_update(port, SC16IS7XX_MCR_REG,
-- 
2.25.1

[PATCH 2/6] sc16is7xx: Update status lines in single call

[Tomasz Moń]

From: Lech Perczak <l.perczak@camlintechnologies.com>

RTS, DTR and LOOP bits can be updated in a single MCR register update.
This reduces the number of (slow) SPI/I2C bus transactions.

Signed-off-by: Lech Perczak <l.perczak@camlintechnologies.com>
Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 3800733452fe..c62531b2efe2 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -787,19 +787,24 @@ static void sc16is7xx_reg_proc(struct kthread_work *ws)
 	spin_unlock_irqrestore(&one->port.lock, irqflags);
 
 	if (config.flags & SC16IS7XX_RECONF_MD) {
+		u8 mcr = 0;
+
+		/* Device ignores RTS setting when hardware flow is enabled */
+		if (one->port.mctrl & TIOCM_RTS)
+			mcr |= SC16IS7XX_MCR_RTS_BIT;
+
+		if (one->port.mctrl & TIOCM_DTR)
+			mcr |= SC16IS7XX_MCR_DTR_BIT;
+
+		if (one->port.mctrl & TIOCM_LOOP)
+			mcr |= SC16IS7XX_MCR_LOOP_BIT;
 		sc16is7xx_port_update(&one->port, SC16IS7XX_MCR_REG,
+				      SC16IS7XX_MCR_RTS_BIT |
+				      SC16IS7XX_MCR_DTR_BIT |
 				      SC16IS7XX_MCR_LOOP_BIT,
-				      (one->port.mctrl & TIOCM_LOOP) ?
-				      SC16IS7XX_MCR_LOOP_BIT : 0);
-		sc16is7xx_port_update(&one->port, SC16IS7XX_MCR_REG,
-				      SC16IS7XX_MCR_RTS_BIT,
-				      (one->port.mctrl & TIOCM_RTS) ?
-				      SC16IS7XX_MCR_RTS_BIT : 0);
-		sc16is7xx_port_update(&one->port, SC16IS7XX_MCR_REG,
-				      SC16IS7XX_MCR_DTR_BIT,
-				      (one->port.mctrl & TIOCM_DTR) ?
-				      SC16IS7XX_MCR_DTR_BIT : 0);
+				      mcr);
 	}
+
 	if (config.flags & SC16IS7XX_RECONF_IER)
 		sc16is7xx_port_update(&one->port, SC16IS7XX_IER_REG,
 				      config.ier_clear, 0);
-- 
2.25.1

[PATCH 3/6] sc16is7xx: Separate GPIOs from modem control lines

[Tomasz Moń]

From: Lech Perczak <l.perczak@camlintechnologies.com>

Export only the GPIOs that are not shared with hardware modem control
lines. Introduce new device parameter indicating whether modem control
lines are available.

Signed-off-by: Lech Perczak <l.perczak@camlintechnologies.com>
Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index c62531b2efe2..21ae2c0b7bbe 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -306,6 +306,7 @@ struct sc16is7xx_devtype {
 	char	name[10];
 	int	nr_gpio;
 	int	nr_uart;
+	int	has_mctrl;
 };
 
 #define SC16IS7XX_RECONF_MD		(1 << 0)
@@ -440,30 +441,35 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
 	.name		= "SC16IS74X",
 	.nr_gpio	= 0,
 	.nr_uart	= 1,
+	.has_mctrl	= 0,
 };
 
 static const struct sc16is7xx_devtype sc16is750_devtype = {
 	.name		= "SC16IS750",
-	.nr_gpio	= 8,
+	.nr_gpio	= 4,
 	.nr_uart	= 1,
+	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is752_devtype = {
 	.name		= "SC16IS752",
-	.nr_gpio	= 8,
+	.nr_gpio	= 0,
 	.nr_uart	= 2,
+	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is760_devtype = {
 	.name		= "SC16IS760",
-	.nr_gpio	= 8,
+	.nr_gpio	= 4,
 	.nr_uart	= 1,
+	.has_mctrl	= 1,
 };
 
 static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.name		= "SC16IS762",
-	.nr_gpio	= 8,
+	.nr_gpio	= 0,
 	.nr_uart	= 2,
+	.has_mctrl	= 1,
 };
 
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
-- 
2.25.1

[PATCH 4/6] sc16is7xx: Properly resume TX after stop

[Tomasz Moń]

sc16is7xx_stop_tx() clears THRI bit and thus disables THRI interrupt.
This makes it possible for transmission to cease indefinitely when more
than 64 characters are being sent.

The sc16is7xx_handle_tx() call executed by sc16is7xx_tx_proc() can send
up to FIFO length (64) characters. If more characters are written to the
output buffer, then the THRI interrupt is needed.

Solve the issue by enabling THRI interrupt in sc16is7xx_tx_proc().

Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 47 +++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 21ae2c0b7bbe..17e79af36604 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -315,7 +315,8 @@ struct sc16is7xx_devtype {
 
 struct sc16is7xx_one_config {
 	unsigned int			flags;
-	u8				ier_clear;
+	u8				ier_mask;
+	u8				ier_val;
 };
 
 struct sc16is7xx_one {
@@ -349,6 +350,9 @@ static struct uart_driver sc16is7xx_uart = {
 	.nr		= SC16IS7XX_MAX_DEVS,
 };
 
+static void sc16is7xx_ier_set(struct uart_port *port, u8 bit);
+static void sc16is7xx_stop_tx(struct uart_port *port);
+
 #define to_sc16is7xx_port(p,e)	((container_of((p), struct sc16is7xx_port, e)))
 #define to_sc16is7xx_one(p,e)	((container_of((p), struct sc16is7xx_one, e)))
 
@@ -651,6 +655,7 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct circ_buf *xmit = &port->state->xmit;
 	unsigned int txlen, to_send, i;
+	unsigned long flags;
 
 	if (unlikely(port->x_char)) {
 		sc16is7xx_port_write(port, SC16IS7XX_THR_REG, port->x_char);
@@ -659,8 +664,12 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 		return;
 	}
 
-	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+		spin_lock_irqsave(&port->lock, flags);
+		sc16is7xx_stop_tx(port);
+		spin_unlock_irqrestore(&port->lock, flags);
 		return;
+	}
 
 	/* Get length of data pending in circular buffer */
 	to_send = uart_circ_chars_pending(xmit);
@@ -687,8 +696,13 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 		sc16is7xx_fifo_write(port, to_send);
 	}
 
+	spin_lock_irqsave(&port->lock, flags);
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
+
+	if (uart_circ_empty(xmit))
+		sc16is7xx_stop_tx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
@@ -751,6 +765,7 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
 {
 	struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port);
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+	unsigned long flags;
 
 	if ((port->rs485.flags & SER_RS485_ENABLED) &&
 	    (port->rs485.delay_rts_before_send > 0))
@@ -759,6 +774,10 @@ static void sc16is7xx_tx_proc(struct kthread_work *ws)
 	mutex_lock(&s->efr_lock);
 	sc16is7xx_handle_tx(port);
 	mutex_unlock(&s->efr_lock);
+
+	spin_lock_irqsave(&port->lock, flags);
+	sc16is7xx_ier_set(port, SC16IS7XX_IER_THRI_BIT);
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static void sc16is7xx_reconf_rs485(struct uart_port *port)
@@ -813,7 +832,7 @@ static void sc16is7xx_reg_proc(struct kthread_work *ws)
 
 	if (config.flags & SC16IS7XX_RECONF_IER)
 		sc16is7xx_port_update(&one->port, SC16IS7XX_IER_REG,
-				      config.ier_clear, 0);
+				      config.ier_mask, config.ier_val);
 
 	if (config.flags & SC16IS7XX_RECONF_RS485)
 		sc16is7xx_reconf_rs485(&one->port);
@@ -824,8 +843,24 @@ static void sc16is7xx_ier_clear(struct uart_port *port, u8 bit)
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
+	lockdep_assert_held_once(&port->lock);
+
+	one->config.flags |= SC16IS7XX_RECONF_IER;
+	one->config.ier_mask |= bit;
+	one->config.ier_val &= ~bit;
+	kthread_queue_work(&s->kworker, &one->reg_work);
+}
+
+static void sc16is7xx_ier_set(struct uart_port *port, u8 bit)
+{
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+	lockdep_assert_held_once(&port->lock);
+
 	one->config.flags |= SC16IS7XX_RECONF_IER;
-	one->config.ier_clear |= bit;
+	one->config.ier_mask |= bit;
+	one->config.ier_val |= bit;
 	kthread_queue_work(&s->kworker, &one->reg_work);
 }
 
@@ -1067,8 +1102,8 @@ static int sc16is7xx_startup(struct uart_port *port)
 			      SC16IS7XX_EFCR_TXDISABLE_BIT,
 			      0);
 
-	/* Enable RX, TX interrupts */
-	val = SC16IS7XX_IER_RDI_BIT | SC16IS7XX_IER_THRI_BIT;
+	/* Enable RX interrupt */
+	val = SC16IS7XX_IER_RDI_BIT;
 	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, val);
 
 	return 0;
-- 
2.25.1

[PATCH 5/6] sc16is7xx: Handle modem status lines

[Tomasz Moń]

The uart_handle_cts_change() and uart_handle_dcd_change() must be called
with port lock being held. Acquire the lock after reading MSR register.
Do not acquire spin lock when reading MSR register because I2C/SPI port
functions cannot be called with spinlocks held.

Update rng and dsr counters. Wake up delta_msr_wait to allow tty notice
modem status change.

Co-developed-by: Lech Perczak <l.perczak@camlintechnologies.com>
Signed-off-by: Lech Perczak <l.perczak@camlintechnologies.com>
Co-developed-by: Tomasz Moń <tomasz.mon@camlingroup.com>
Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 120 +++++++++++++++++++++++++++++++--
 1 file changed, 114 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 17e79af36604..5c247b4a01a9 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -324,8 +324,10 @@ struct sc16is7xx_one {
 	u8				line;
 	struct kthread_work		tx_work;
 	struct kthread_work		reg_work;
+	struct kthread_delayed_work	ms_work;
 	struct sc16is7xx_one_config	config;
 	bool				irda_mode;
+	unsigned int			old_mctrl;
 };
 
 struct sc16is7xx_port {
@@ -705,12 +707,56 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
+static unsigned int sc16is7xx_get_hwmctrl(struct uart_port *port)
+{
+	u8 msr = sc16is7xx_port_read(port, SC16IS7XX_MSR_REG);
+	unsigned int mctrl = 0;
+
+	mctrl |= (msr & SC16IS7XX_MSR_CTS_BIT) ? TIOCM_CTS : 0;
+	mctrl |= (msr & SC16IS7XX_MSR_DSR_BIT) ? TIOCM_DSR : 0;
+	mctrl |= (msr & SC16IS7XX_MSR_CD_BIT)  ? TIOCM_CAR : 0;
+	mctrl |= (msr & SC16IS7XX_MSR_RI_BIT)  ? TIOCM_RNG : 0;
+	return mctrl;
+}
+
+static void sc16is7xx_update_mlines(struct sc16is7xx_one *one)
+{
+	struct uart_port *port = &one->port;
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+	unsigned long flags;
+	unsigned int status, changed;
+
+	lockdep_assert_held_once(&s->efr_lock);
+
+	status = sc16is7xx_get_hwmctrl(port);
+	changed = status ^ one->old_mctrl;
+
+	if (changed == 0)
+		return;
+
+	one->old_mctrl = status;
+
+	spin_lock_irqsave(&port->lock, flags);
+	if ((changed & TIOCM_RNG) && (status & TIOCM_RNG))
+		port->icount.rng++;
+	if (changed & TIOCM_DSR)
+		port->icount.dsr++;
+	if (changed & TIOCM_CAR)
+		uart_handle_dcd_change(port, status & TIOCM_CAR);
+	if (changed & TIOCM_CTS)
+		uart_handle_cts_change(port, status & TIOCM_CTS);
+
+	wake_up_interruptible(&port->state->port.delta_msr_wait);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
 static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 {
 	struct uart_port *port = &s->p[portno].port;
 
 	do {
 		unsigned int iir, rxlen;
+		struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 
 		iir = sc16is7xx_port_read(port, SC16IS7XX_IIR_REG);
 		if (iir & SC16IS7XX_IIR_NO_INT_BIT)
@@ -727,6 +773,11 @@ static bool sc16is7xx_port_irq(struct sc16is7xx_port *s, int portno)
 			if (rxlen)
 				sc16is7xx_handle_rx(port, rxlen, iir);
 			break;
+		/* CTSRTS interrupt comes only when CTS goes inactive */
+		case SC16IS7XX_IIR_CTSRTS_SRC:
+		case SC16IS7XX_IIR_MSI_SRC:
+			sc16is7xx_update_mlines(one);
+			break;
 		case SC16IS7XX_IIR_THRI_SRC:
 			sc16is7xx_handle_tx(port);
 			break;
@@ -874,6 +925,30 @@ static void sc16is7xx_stop_rx(struct uart_port *port)
 	sc16is7xx_ier_clear(port, SC16IS7XX_IER_RDI_BIT);
 }
 
+static void sc16is7xx_ms_proc(struct kthread_work *ws)
+{
+	struct sc16is7xx_one *one = to_sc16is7xx_one(ws, ms_work.work);
+	struct sc16is7xx_port *s = dev_get_drvdata(one->port.dev);
+
+	if (one->port.state) {
+		mutex_lock(&s->efr_lock);
+		sc16is7xx_update_mlines(one);
+		mutex_unlock(&s->efr_lock);
+
+		kthread_queue_delayed_work(&s->kworker, &one->ms_work, HZ);
+	}
+}
+
+static void sc16is7xx_enable_ms(struct uart_port *port)
+{
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+
+	lockdep_assert_held_once(&port->lock);
+
+	kthread_queue_delayed_work(&s->kworker, &one->ms_work, 0);
+}
+
 static void sc16is7xx_start_tx(struct uart_port *port)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
@@ -893,10 +968,10 @@ static unsigned int sc16is7xx_tx_empty(struct uart_port *port)
 
 static unsigned int sc16is7xx_get_mctrl(struct uart_port *port)
 {
-	/* DCD and DSR are not wired and CTS/RTS is handled automatically
-	 * so just indicate DSR and CAR asserted
-	 */
-	return TIOCM_DSR | TIOCM_CAR;
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+	/* Called with port lock taken so we can only return cached value */
+	return one->old_mctrl;
 }
 
 static void sc16is7xx_set_mctrl(struct uart_port *port, unsigned int mctrl)
@@ -920,8 +995,12 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 				  struct ktermios *old)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 	unsigned int lcr, flow = 0;
 	int baud;
+	unsigned long flags;
+
+	kthread_cancel_delayed_work_sync(&one->ms_work);
 
 	/* Mask termios capabilities we don't support */
 	termios->c_cflag &= ~CMSPAR;
@@ -1010,8 +1089,15 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	/* Setup baudrate generator */
 	baud = sc16is7xx_set_baud(port, baud);
 
+	spin_lock_irqsave(&port->lock, flags);
+
 	/* Update timeout according to new baud rate */
 	uart_update_timeout(port, termios->c_cflag, baud);
+
+	if (UART_ENABLE_MS(port, termios->c_cflag))
+		sc16is7xx_enable_ms(port);
+
+	spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static int sc16is7xx_config_rs485(struct uart_port *port,
@@ -1052,6 +1138,7 @@ static int sc16is7xx_startup(struct uart_port *port)
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	unsigned int val;
+	unsigned long flags;
 
 	sc16is7xx_power(port, 1);
 
@@ -1102,16 +1189,25 @@ static int sc16is7xx_startup(struct uart_port *port)
 			      SC16IS7XX_EFCR_TXDISABLE_BIT,
 			      0);
 
-	/* Enable RX interrupt */
-	val = SC16IS7XX_IER_RDI_BIT;
+	/* Enable RX, CTS change and modem lines interrupts */
+	val = SC16IS7XX_IER_RDI_BIT | SC16IS7XX_IER_CTSI_BIT |
+	      SC16IS7XX_IER_MSI_BIT;
 	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, val);
 
+	/* Enable modem status polling */
+	spin_lock_irqsave(&port->lock, flags);
+	sc16is7xx_enable_ms(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	return 0;
 }
 
 static void sc16is7xx_shutdown(struct uart_port *port)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
+	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+
+	kthread_cancel_delayed_work_sync(&one->ms_work);
 
 	/* Disable all interrupts */
 	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, 0);
@@ -1175,6 +1271,7 @@ static const struct uart_ops sc16is7xx_ops = {
 	.stop_tx	= sc16is7xx_stop_tx,
 	.start_tx	= sc16is7xx_start_tx,
 	.stop_rx	= sc16is7xx_stop_rx,
+	.enable_ms	= sc16is7xx_enable_ms,
 	.break_ctl	= sc16is7xx_break_ctl,
 	.startup	= sc16is7xx_startup,
 	.shutdown	= sc16is7xx_shutdown,
@@ -1341,7 +1438,9 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
 		s->p[i].port.ops	= &sc16is7xx_ops;
+		s->p[i].old_mctrl	= 0;
 		s->p[i].port.line	= sc16is7xx_alloc_line();
+
 		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
 			ret = -ENOMEM;
 			goto out_ports;
@@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
 				     SC16IS7XX_EFCR_RXDISABLE_BIT |
 				     SC16IS7XX_EFCR_TXDISABLE_BIT);
+
+		/* Use GPIO lines as modem status registers */
+		if (devtype->has_mctrl)
+			sc16is7xx_port_write(&s->p[i].port,
+					     SC16IS7XX_IOCONTROL_REG,
+					     SC16IS7XX_IOCONTROL_MODEM_BIT);
+
 		/* Initialize kthread work structs */
 		kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
 		kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
+		kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
 		/* Register port */
 		uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
 
@@ -1439,6 +1546,7 @@ static void sc16is7xx_remove(struct device *dev)
 #endif
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
+		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
 		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		clear_bit(s->p[i].port.line, &sc16is7xx_lines);
 		sc16is7xx_power(&s->p[i].port, 0);
-- 
2.25.1

[PATCH 6/6] sc16is7xx: Set AUTOCTS and AUTORTS bits

[Tomasz Moń]

Let serial core know that the chip automatically handles RTS/CTS signal.
This elimines completely unnecessary I2C/SPI bus traffic.

Cease reading from RX FIFO (by disabling RDI interrupt) when throttled.
Eventually the FIFO will fill up and the device will drive RTS output
inactive. Unthrottle by enabling back RDI interrupt.

Indirectly controlling RTS via RX FIFO state seems to be the only option
because RTS bit is ignored when hardware flow control is enabled.

Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5c247b4a01a9..683dd3be010d 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -957,6 +957,29 @@ static void sc16is7xx_start_tx(struct uart_port *port)
 	kthread_queue_work(&s->kworker, &one->tx_work);
 }
 
+static void sc16is7xx_throttle(struct uart_port *port)
+{
+	unsigned long flags;
+
+	/*
+	 * Hardware flow control is enabled and thus the device ignores RTS
+	 * value set in MCR register. Stop reading data from RX FIFO so the
+	 * AutoRTS feature will de-activate RTS output.
+	 */
+	spin_lock_irqsave(&port->lock, flags);
+	sc16is7xx_ier_clear(port, SC16IS7XX_IER_RDI_BIT);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void sc16is7xx_unthrottle(struct uart_port *port)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	sc16is7xx_ier_set(port, SC16IS7XX_IER_RDI_BIT);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
 static unsigned int sc16is7xx_tx_empty(struct uart_port *port)
 {
 	unsigned int lsr;
@@ -1062,9 +1085,13 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	regcache_cache_bypass(s->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_XON1_REG, termios->c_cc[VSTART]);
 	sc16is7xx_port_write(port, SC16IS7XX_XOFF1_REG, termios->c_cc[VSTOP]);
-	if (termios->c_cflag & CRTSCTS)
+
+	port->status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS);
+	if (termios->c_cflag & CRTSCTS) {
 		flow |= SC16IS7XX_EFR_AUTOCTS_BIT |
 			SC16IS7XX_EFR_AUTORTS_BIT;
+		port->status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
+	}
 	if (termios->c_iflag & IXON)
 		flow |= SC16IS7XX_EFR_SWFLOW3_BIT;
 	if (termios->c_iflag & IXOFF)
@@ -1270,6 +1297,8 @@ static const struct uart_ops sc16is7xx_ops = {
 	.get_mctrl	= sc16is7xx_get_mctrl,
 	.stop_tx	= sc16is7xx_stop_tx,
 	.start_tx	= sc16is7xx_start_tx,
+	.throttle	= sc16is7xx_throttle,
+	.unthrottle	= sc16is7xx_unthrottle,
 	.stop_rx	= sc16is7xx_stop_rx,
 	.enable_ms	= sc16is7xx_enable_ms,
 	.break_ctl	= sc16is7xx_break_ctl,
-- 
2.25.1

Re: [PATCH 0/6] sc16is7xx: Hardware flow control fixes

[Greg Kroah-Hartman]

On Mon, Feb 21, 2022 at 11:56:12AM +0100, Tomasz Moń wrote:
> sc16is7xx driver assumes that the device handles hardware flow control
> automatically. This is not really true as the driver does inadvertently
> clear the bits that enable hardware flow control.
> 
> This patch series solves multiple issues present in the driver. While
> the patches are fairly independent, there are some dependencies. The
> "sc16is7xx: Properly resume TX after stop" adds IER bit set function
> that is later used in "sc16is7xx: Set AUTOCTS and AUTORTS bits". Also
> the patches that control which interrupts are enabled are dependent on
> each other.
> 
> Patches should be applied respecting the order in the series. The whole
> series applies on top of "sc16is7xx: Fix for incorrect data being
> transmitted" [1].

The first 3 patches of this series applied.  Please rebase and resend
the remaining.

thanks,

greg k-h

Re: [PATCH 0/6] sc16is7xx: Hardware flow control fixes

[Tomasz Moń]

On 25.02.2022 10:25, Greg Kroah-Hartman wrote:
> On Mon, Feb 21, 2022 at 11:56:12AM +0100, Tomasz Moń wrote:
>> sc16is7xx driver assumes that the device handles hardware flow control
>> automatically. This is not really true as the driver does inadvertently
>> clear the bits that enable hardware flow control.
>>
>> This patch series solves multiple issues present in the driver. While
>> the patches are fairly independent, there are some dependencies. The
>> "sc16is7xx: Properly resume TX after stop" adds IER bit set function
>> that is later used in "sc16is7xx: Set AUTOCTS and AUTORTS bits". Also
>> the patches that control which interrupts are enabled are dependent on
>> each other.
>>
>> Patches should be applied respecting the order in the series. The whole
>> series applies on top of "sc16is7xx: Fix for incorrect data being
>> transmitted" [1].
> 
> The first 3 patches of this series applied.  Please rebase and resend
> the remaining.

The remaining patches did not apply because the "sc16is7xx: Fix for
incorrect data being transmitted" by Phil Elwell was not applied.

The Phil Elwell patch was independently developed and made it to the
list before I sent the patch series. For that reason I based the series
on top of that patch and mentioned it in the cover letter.

I am unsure what is the correct method when handling such situations.
Should I include the Phil Elwell patch when resending the patch series?

Best Regards,
Tomasz Mon

Re: [PATCH 0/6] sc16is7xx: Hardware flow control fixes

[Greg Kroah-Hartman]

On Fri, Feb 25, 2022 at 10:37:01AM +0100, Tomasz Moń wrote:
> On 25.02.2022 10:25, Greg Kroah-Hartman wrote:
> > On Mon, Feb 21, 2022 at 11:56:12AM +0100, Tomasz Moń wrote:
> >> sc16is7xx driver assumes that the device handles hardware flow control
> >> automatically. This is not really true as the driver does inadvertently
> >> clear the bits that enable hardware flow control.
> >>
> >> This patch series solves multiple issues present in the driver. While
> >> the patches are fairly independent, there are some dependencies. The
> >> "sc16is7xx: Properly resume TX after stop" adds IER bit set function
> >> that is later used in "sc16is7xx: Set AUTOCTS and AUTORTS bits". Also
> >> the patches that control which interrupts are enabled are dependent on
> >> each other.
> >>
> >> Patches should be applied respecting the order in the series. The whole
> >> series applies on top of "sc16is7xx: Fix for incorrect data being
> >> transmitted" [1].
> > 
> > The first 3 patches of this series applied.  Please rebase and resend
> > the remaining.
> 
> The remaining patches did not apply because the "sc16is7xx: Fix for
> incorrect data being transmitted" by Phil Elwell was not applied.
> 
> The Phil Elwell patch was independently developed and made it to the
> list before I sent the patch series. For that reason I based the series
> on top of that patch and mentioned it in the cover letter.

If that patch was not applied, then just rebase your series on my branch
and resend it.

thanks,

greg k-h

Re: [PATCH 0/6] sc16is7xx: Hardware flow control fixes

[Greg Kroah-Hartman]

On Fri, Feb 25, 2022 at 11:47:04AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 25, 2022 at 10:37:01AM +0100, Tomasz Moń wrote:
> > On 25.02.2022 10:25, Greg Kroah-Hartman wrote:
> > > On Mon, Feb 21, 2022 at 11:56:12AM +0100, Tomasz Moń wrote:
> > >> sc16is7xx driver assumes that the device handles hardware flow control
> > >> automatically. This is not really true as the driver does inadvertently
> > >> clear the bits that enable hardware flow control.
> > >>
> > >> This patch series solves multiple issues present in the driver. While
> > >> the patches are fairly independent, there are some dependencies. The
> > >> "sc16is7xx: Properly resume TX after stop" adds IER bit set function
> > >> that is later used in "sc16is7xx: Set AUTOCTS and AUTORTS bits". Also
> > >> the patches that control which interrupts are enabled are dependent on
> > >> each other.
> > >>
> > >> Patches should be applied respecting the order in the series. The whole
> > >> series applies on top of "sc16is7xx: Fix for incorrect data being
> > >> transmitted" [1].
> > > 
> > > The first 3 patches of this series applied.  Please rebase and resend
> > > the remaining.
> > 
> > The remaining patches did not apply because the "sc16is7xx: Fix for
> > incorrect data being transmitted" by Phil Elwell was not applied.
> > 
> > The Phil Elwell patch was independently developed and made it to the
> > list before I sent the patch series. For that reason I based the series
> > on top of that patch and mentioned it in the cover letter.
> 
> If that patch was not applied, then just rebase your series on my branch
> and resend it.

Ah, Phil's patch is in a different branch, and will be sent to Linus
later today, so you might just want to wait until Monday to resend your
remaining patches and then they will apply as my tty-next branch will
merge with that at that point in time.

thanks,

greg k-h