[PATCH v3 00/12] Add RS485 support to DW UART

[PATCH v3 00/12] Add RS485 support to DW UART

[Ilpo Järvinen]

This patchset adds RS-485 support to the DW UART driver. The patchset
has two main parts. The first part adds HW support for RS-485 itself
in various modes of operation and the second part focuses on enabling
9th bit addressing mode that can be used on a multipoint RS-485
communications line.

To configure multipoint addressing, ADDRB flag is added to termios
and two new IOCTLs are added into serial core. Lukas Wunner brought up
during v1 review that if this addressing is only going to be used with
RS-485, doing it within rs485_config would avoid having to add those
IOCTLs. There was some counterexample w/o further details mentioned for
RS-232 usage by Andy Shevchenko. I left the IOCTL approach there but if
somebody has further input on this, please voice it as it is user-space
facing API.

I decided to rewrite the UART_CAP_NOTEMT patch from scratch myself
based on Uwe Kleine-König's earlier suggestion and include it to this
series. To make waiting for a single character easy and to avoid
storing it per purpose in the uart drivers, I decided to add
frame_time into uart_port. It turned out to beneficial also for serial
core which had to reverse calculate it from uart_port->timeout). I was
thinking of removing uart_port->timeout entirely and derive the value
timeout from frame_time and fifosize where needed but I was not sure
if that's ok to do lockingwise (not that fifosize is a variable that
is expected to change so maybe I'm just being too cautious).

Cc: linux-api@vger.kernel.org

v1 -> v2:
- Add uart_port->frame_time to avoid the need to store it per purpose
- Included NOTEMT patch rewritten from scratch
- Merge HW half & full-duplex patches
- Detect RS485 HW using RE_EN register write+read
- Removed SER_RS485_SW_RX_OR_TX
- Relocated/renamed RE polarity DT prop
- Use SER_RS485_RTS_ON_SEND rather than DT prop directly
- Removed DE polarity prop, it is still configurable but with rts one instead
- Make DE active-high by default in dwlib
- Don't unnecessarily clear DE/RE_EN for non-RS485 mode
- Prevent ADDRB and addrmode desync for RS485->RS232 transition
- Added ACPI enumeration doc
- Changed -EINVAL to -ENOTTY if no set/get_addr handler is present
- Clear ADDRB in set_termios of a few more drivers
- Added filtering for addresses to avoid them leaking into data stream
- Reworded comments & commit messages as requested

v2 -> v3
- Change ADDRB to 0x20000000 which is free for all archs
- Added TIOCSADDR/GADDR to tty_compat_ioctl

Ilpo Järvinen (12):
  serial: Store character timing information to uart_port
  serial: 8250: Handle UART without interrupt on TEMT
  serial: 8250_dwlib: RS485 HW half & full duplex support
  serial: 8250_dwlib: Implement SW half duplex support
  dt_bindings: rs485: Add receiver enable polarity
  ACPI / property: Document RS485 _DSD properties
  serial: termbits: ADDRB to indicate 9th bit addressing mode
  serial: General support for multipoint addresses
  serial: 8250: make saved LSR larger
  serial: 8250: create lsr_save_mask
  serial: 8250_lpss: Use 32-bit reads
  serial: 8250_dwlib: Support for 9th bit multipoint addressing

 .../devicetree/bindings/serial/rs485.yaml     |   5 +
 .../driver-api/serial/serial-rs485.rst        |  23 +-
 .../firmware-guide/acpi/enumeration.rst       |  25 ++
 arch/alpha/include/uapi/asm/ioctls.h          |   3 +
 arch/alpha/include/uapi/asm/termbits.h        |   1 +
 arch/mips/include/uapi/asm/ioctls.h           |   3 +
 arch/mips/include/uapi/asm/termbits.h         |   1 +
 arch/parisc/include/uapi/asm/ioctls.h         |   3 +
 arch/parisc/include/uapi/asm/termbits.h       |   1 +
 arch/powerpc/include/uapi/asm/ioctls.h        |   3 +
 arch/powerpc/include/uapi/asm/termbits.h      |   1 +
 arch/sh/include/uapi/asm/ioctls.h             |   3 +
 arch/sparc/include/uapi/asm/ioctls.h          |   3 +
 arch/sparc/include/uapi/asm/termbits.h        |   1 +
 arch/xtensa/include/uapi/asm/ioctls.h         |   3 +
 drivers/char/pcmcia/synclink_cs.c             |   2 +
 drivers/ipack/devices/ipoctal.c               |   2 +
 drivers/mmc/core/sdio_uart.c                  |   2 +
 drivers/net/usb/hso.c                         |   3 +-
 drivers/s390/char/tty3270.c                   |   3 +
 drivers/staging/greybus/uart.c                |   2 +
 drivers/tty/amiserial.c                       |   6 +-
 drivers/tty/moxa.c                            |   1 +
 drivers/tty/mxser.c                           |   1 +
 drivers/tty/serial/8250/8250.h                |   1 +
 drivers/tty/serial/8250/8250_core.c           |   6 +-
 drivers/tty/serial/8250/8250_dwlib.c          | 232 +++++++++++++++++-
 drivers/tty/serial/8250/8250_dwlib.h          |   5 +
 drivers/tty/serial/8250/8250_lpss.c           |   2 +-
 drivers/tty/serial/8250/8250_port.c           |  42 ++--
 drivers/tty/serial/serial_core.c              |  76 +++++-
 drivers/tty/synclink_gt.c                     |   2 +
 drivers/tty/tty_io.c                          |   2 +
 drivers/tty/tty_ioctl.c                       |   2 +
 drivers/usb/class/cdc-acm.c                   |   2 +
 drivers/usb/serial/usb-serial.c               |   6 +-
 include/linux/serial_8250.h                   |   7 +-
 include/linux/serial_core.h                   |   7 +
 include/uapi/asm-generic/ioctls.h             |   3 +
 include/uapi/asm-generic/termbits.h           |   1 +
 include/uapi/linux/serial.h                   |   8 +
 net/bluetooth/rfcomm/tty.c                    |   2 +
 42 files changed, 472 insertions(+), 35 deletions(-)

-- 
2.30.2

[PATCH v3 07/12] serial: termbits: ADDRB to indicate 9th bit addressing mode

[Ilpo Järvinen]

Add ADDRB to termbits to indicate 9th bit addressing mode.
This change is necessary for supporting devices with RS485
multipoint addressing [*]. A later patch in the patch series
adds support for Synopsys Designware UART capable for 9th bit
addressing mode. In this mode, 9th bit is used to indicate an
address (byte) within the communication line. The 9th bit
addressing mode is selected using ADDRB introduced by an earlier
patch.

[*] Technically, RS485 is just an electronic spec and does not
itself specify the 9th bit addressing mode but 9th bit seems
at least "semi-standard" way to do addressing with RS485.

Cc: linux-api@vger.kernel.org
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-mips@vger.kernel.org
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/alpha/include/uapi/asm/termbits.h   | 1 +
 arch/mips/include/uapi/asm/termbits.h    | 1 +
 arch/parisc/include/uapi/asm/termbits.h  | 1 +
 arch/powerpc/include/uapi/asm/termbits.h | 1 +
 arch/sparc/include/uapi/asm/termbits.h   | 1 +
 drivers/char/pcmcia/synclink_cs.c        | 2 ++
 drivers/ipack/devices/ipoctal.c          | 2 ++
 drivers/mmc/core/sdio_uart.c             | 2 ++
 drivers/net/usb/hso.c                    | 3 ++-
 drivers/s390/char/tty3270.c              | 3 +++
 drivers/staging/greybus/uart.c           | 2 ++
 drivers/tty/amiserial.c                  | 6 +++++-
 drivers/tty/moxa.c                       | 1 +
 drivers/tty/mxser.c                      | 1 +
 drivers/tty/serial/serial_core.c         | 2 ++
 drivers/tty/synclink_gt.c                | 2 ++
 drivers/tty/tty_ioctl.c                  | 2 ++
 drivers/usb/class/cdc-acm.c              | 2 ++
 drivers/usb/serial/usb-serial.c          | 6 ++++--
 include/uapi/asm-generic/termbits.h      | 1 +
 net/bluetooth/rfcomm/tty.c               | 2 ++
 21 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/arch/alpha/include/uapi/asm/termbits.h b/arch/alpha/include/uapi/asm/termbits.h
index 4575ba34a0ea..0c123e715486 100644
--- a/arch/alpha/include/uapi/asm/termbits.h
+++ b/arch/alpha/include/uapi/asm/termbits.h
@@ -180,6 +180,7 @@ struct ktermios {
 #define HUPCL	00040000
 
 #define CLOCAL	00100000
+#define ADDRB	004000000000		/* address bit */
 #define CMSPAR	  010000000000		/* mark or space (stick) parity */
 #define CRTSCTS	  020000000000		/* flow control */
 
diff --git a/arch/mips/include/uapi/asm/termbits.h b/arch/mips/include/uapi/asm/termbits.h
index dfeffba729b7..4732d31b0e4e 100644
--- a/arch/mips/include/uapi/asm/termbits.h
+++ b/arch/mips/include/uapi/asm/termbits.h
@@ -182,6 +182,7 @@ struct ktermios {
 #define	 B3500000 0010016
 #define	 B4000000 0010017
 #define CIBAUD	  002003600000	/* input baud rate */
+#define ADDRB	  004000000000	/* address bit */
 #define CMSPAR	  010000000000	/* mark or space (stick) parity */
 #define CRTSCTS	  020000000000	/* flow control */
 
diff --git a/arch/parisc/include/uapi/asm/termbits.h b/arch/parisc/include/uapi/asm/termbits.h
index 40e920f8d683..d6bbd10d92ba 100644
--- a/arch/parisc/include/uapi/asm/termbits.h
+++ b/arch/parisc/include/uapi/asm/termbits.h
@@ -159,6 +159,7 @@ struct ktermios {
 #define  B3500000 0010016
 #define  B4000000 0010017
 #define CIBAUD    002003600000		/* input baud rate */
+#define ADDRB	  004000000000		/* address bit */
 #define CMSPAR    010000000000          /* mark or space (stick) parity */
 #define CRTSCTS   020000000000          /* flow control */
 
diff --git a/arch/powerpc/include/uapi/asm/termbits.h b/arch/powerpc/include/uapi/asm/termbits.h
index ed18bc61f63d..c6a033732f39 100644
--- a/arch/powerpc/include/uapi/asm/termbits.h
+++ b/arch/powerpc/include/uapi/asm/termbits.h
@@ -171,6 +171,7 @@ struct ktermios {
 #define HUPCL	00040000
 
 #define CLOCAL	00100000
+#define ADDRB	004000000000		/* address bit */
 #define CMSPAR	  010000000000		/* mark or space (stick) parity */
 #define CRTSCTS	  020000000000		/* flow control */
 
diff --git a/arch/sparc/include/uapi/asm/termbits.h b/arch/sparc/include/uapi/asm/termbits.h
index ce5ad5d0f105..5eb1d547b5c4 100644
--- a/arch/sparc/include/uapi/asm/termbits.h
+++ b/arch/sparc/include/uapi/asm/termbits.h
@@ -201,6 +201,7 @@ struct ktermios {
 #define B3500000  0x00001012
 #define B4000000  0x00001013  */
 #define CIBAUD	  0x100f0000  /* input baud rate (not used) */
+#define ADDRB	  0x20000000  /* address bit */
 #define CMSPAR	  0x40000000  /* mark or space (stick) parity */
 #define CRTSCTS	  0x80000000  /* flow control */
 
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 78baba55a8b5..d179b9b57a25 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -2287,6 +2287,8 @@ static void mgslpc_set_termios(struct tty_struct *tty, struct ktermios *old_term
 		== RELEVANT_IFLAG(old_termios->c_iflag)))
 	  return;
 
+	tty->termios.c_cflag &= ~ADDRB;
+
 	mgslpc_change_params(info, tty);
 
 	/* Handle transition to B0 status */
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index 20d2b9ec1227..d66cc9683ebc 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -506,6 +506,8 @@ static void ipoctal_set_termios(struct tty_struct *tty,
 	struct ipoctal_channel *channel = tty->driver_data;
 	speed_t baud;
 
+	tty->termios.c_cflag &= ~ADDRB;
+
 	cflag = tty->termios.c_cflag;
 
 	/* Disable and reset everything before change the setup */
diff --git a/drivers/mmc/core/sdio_uart.c b/drivers/mmc/core/sdio_uart.c
index 04c0823e0359..7432b01379ef 100644
--- a/drivers/mmc/core/sdio_uart.c
+++ b/drivers/mmc/core/sdio_uart.c
@@ -880,6 +880,8 @@ static void sdio_uart_set_termios(struct tty_struct *tty,
 	if (sdio_uart_claim_func(port) != 0)
 		return;
 
+	tty->termios.c_cflag &= ~ADDRB;
+
 	sdio_uart_change_speed(port, &tty->termios, old_termios);
 
 	/* Handle transition to B0 status */
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index f97813a4e8d1..b687327bc7b1 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1099,7 +1099,8 @@ static void _hso_serial_set_termios(struct tty_struct *tty)
 		~(CSIZE		/* no size */
 		| PARENB	/* disable parity bit */
 		| CBAUD		/* clear current baud rate */
-		| CBAUDEX);	/* clear current buad rate */
+		| CBAUDEX	/* clear current baud rate */
+		| ADDRB);	/* disable 9th (addr) bit */
 
 	tty->termios.c_cflag |= CS8;	/* character size 8 bits */
 
diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 5c83f71c1d0e..253d2997a1d3 100644
--- a/drivers/s390/char/tty3270.c
+++ b/drivers/s390/char/tty3270.c
@@ -1768,6 +1768,9 @@ tty3270_set_termios(struct tty_struct *tty, struct ktermios *old)
 	tp = tty->driver_data;
 	if (!tp)
 		return;
+
+	tty->termios.c_cflag &= ~ADDRB;
+
 	spin_lock_bh(&tp->view.lock);
 	if (L_ICANON(tty)) {
 		new = L_ECHO(tty) ? TF_INPUT: TF_INPUTN;
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
index dc4ed0ff1ae2..83e73aefde0f 100644
--- a/drivers/staging/greybus/uart.c
+++ b/drivers/staging/greybus/uart.c
@@ -487,6 +487,8 @@ static void gb_tty_set_termios(struct tty_struct *tty,
 	struct ktermios *termios = &tty->termios;
 	u8 newctrl = gb_tty->ctrlout;
 
+	termios->c_cflag &= ~ADDRB;
+
 	newline.rate = cpu_to_le32(tty_get_baud_rate(tty));
 	newline.format = termios->c_cflag & CSTOPB ?
 				GB_SERIAL_2_STOP_BITS : GB_SERIAL_1_STOP_BITS;
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 533d02b38e02..3ca97007bd6e 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -1175,7 +1175,11 @@ static void rs_set_termios(struct tty_struct *tty, struct ktermios *old_termios)
 {
 	struct serial_state *info = tty->driver_data;
 	unsigned long flags;
-	unsigned int cflag = tty->termios.c_cflag;
+	unsigned int cflag;
+
+	tty->termios.c_cflag &= ~ADDRB;
+
+	cflag = tty->termios.c_cflag;
 
 	change_speed(tty, info, old_termios);
 
diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index f3c72ab1476c..07cd88152d58 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -2050,6 +2050,7 @@ static int MoxaPortSetTermio(struct moxa_port *port, struct ktermios *termio,
 
 	ofsAddr = port->tableAddr;
 
+	termio->c_cflag &= ~ADDRB;
 	mode = termio->c_cflag & CSIZE;
 	if (mode == CS5)
 		mode = MX_CS5;
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 836c9eca2946..220676363a07 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -577,6 +577,7 @@ static void mxser_change_speed(struct tty_struct *tty, struct ktermios *old_term
 	struct mxser_port *info = tty->driver_data;
 	unsigned cflag, cval;
 
+	tty->termios.c_cflag &= ~ADDRB;
 	cflag = tty->termios.c_cflag;
 
 	if (mxser_set_baud(tty, tty_get_baud_rate(tty))) {
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c6ac91033e38..de198c2acefe 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1493,6 +1493,8 @@ static void uart_set_termios(struct tty_struct *tty,
 		goto out;
 	}
 
+	tty->termios.c_cflag &= ~ADDRB;
+
 	uart_change_speed(tty, state, old_termios);
 	/* reload cflag from termios; port driver may have overridden flags */
 	cflag = tty->termios.c_cflag;
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 25c558e65ece..ee767cea18ed 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -714,6 +714,8 @@ static void set_termios(struct tty_struct *tty, struct ktermios *old_termios)
 
 	DBGINFO(("%s set_termios\n", tty->driver->name));
 
+	tty->termios.c_cflag &= ~ADDRB;
+
 	change_params(info);
 
 	/* Handle transition to B0 status */
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 63181925ec1a..934037d78868 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -319,6 +319,8 @@ unsigned char tty_get_frame_size(unsigned int cflag)
 		bits++;
 	if (cflag & PARENB)
 		bits++;
+	if (cflag & ADDRB)
+		bits++;
 
 	return bits;
 }
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 9b9aea24d58c..fd246ec70da8 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1056,6 +1056,8 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	struct usb_cdc_line_coding newline;
 	int newctrl = acm->ctrlout;
 
+	termios->c_cflag &= ~ADDRB;
+
 	newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
 	newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
 	newline.bParityType = termios->c_cflag & PARENB ?
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 24101bd7fcad..8d1d170eb7e6 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -525,10 +525,12 @@ static void serial_set_termios(struct tty_struct *tty, struct ktermios *old)
 
 	dev_dbg(&port->dev, "%s\n", __func__);
 
-	if (port->serial->type->set_termios)
+	if (port->serial->type->set_termios) {
+		tty->termios.c_cflag &= ~ADDRB;
 		port->serial->type->set_termios(tty, port, old);
-	else
+	} else {
 		tty_termios_copy_hw(&tty->termios, old);
+	}
 }
 
 static int serial_break(struct tty_struct *tty, int break_state)
diff --git a/include/uapi/asm-generic/termbits.h b/include/uapi/asm-generic/termbits.h
index 2fbaf9ae89dd..e06eaa9cf8be 100644
--- a/include/uapi/asm-generic/termbits.h
+++ b/include/uapi/asm-generic/termbits.h
@@ -158,6 +158,7 @@ struct ktermios {
 #define  B3500000 0010016
 #define  B4000000 0010017
 #define CIBAUD	  002003600000	/* input baud rate */
+#define ADDRB	  004000000000	/* address bit */
 #define CMSPAR	  010000000000	/* mark or space (stick) parity */
 #define CRTSCTS	  020000000000	/* flow control */
 
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index ebd78fdbd6e8..832e725f23ab 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -871,6 +871,8 @@ static void rfcomm_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 	if (!dev || !dev->dlc || !dev->dlc->session)
 		return;
 
+	new->c_cflag &= ~ADDRB;
+
 	/* Handle turning off CRTSCTS */
 	if ((old->c_cflag & CRTSCTS) && !(new->c_cflag & CRTSCTS))
 		BT_DBG("Turning off CRTSCTS unsupported");
-- 
2.30.2

[PATCH v3 08/12] serial: General support for multipoint addresses

[Ilpo Järvinen]

Add generic support for serial multipoint addressing. Two new
ioctls are added. TIOCSADDR is used to indicate the
destination/receive address. TIOCGADDR returns the current
address in use. The driver should implement set_addr and get_addr
to support addressing mode.

Adjust ADDRB clearing to happen only if driver does not provide
set_addr (=the driver doesn't support address mode).

This change is necessary for supporting devices with RS485
multipoint addressing [*]. A following patch in the patch series
adds support for Synopsys Designware UART capable for 9th bit
addressing mode. In this mode, 9th bit is used to indicate an
address (byte) within the communication line. The 9th bit
addressing mode is selected using ADDRB introduced by the
previous patch.

Transmit addresses / receiver filter are specified by setting
the flags SER_ADDR_DEST and/or SER_ADDR_RECV. When the user
supplies the transmit address, in the 9bit addressing mode it is
sent out immediately with the 9th bit set to 1. After that, the
subsequent normal data bytes are sent with 9th bit as 0 and they
are intended to the device with the given address. It is up to
receiver to enforce the filter using SER_ADDR_RECV. When userspace
has supplied the receive address, the driver is expected to handle
the matching of the address and only data with that address is
forwarded to the user. Both SER_ADDR_DEST and SER_ADDR_RECV can
be given at the same time in a single call if the addresses are
the same.

The user can clear the receive filter with SER_ADDR_RECV_CLEAR.

[*] Technically, RS485 is just an electronic spec and does not
itself specify the 9th bit addressing mode but 9th bit seems
at least "semi-standard" way to do addressing with RS485.

Cc: linux-api@vger.kernel.org
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-mips@vger.kernel.org
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: linux-sh@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: linux-xtensa@linux-xtensa.org
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../driver-api/serial/serial-rs485.rst        | 23 ++++++-
 arch/alpha/include/uapi/asm/ioctls.h          |  3 +
 arch/mips/include/uapi/asm/ioctls.h           |  3 +
 arch/parisc/include/uapi/asm/ioctls.h         |  3 +
 arch/powerpc/include/uapi/asm/ioctls.h        |  3 +
 arch/sh/include/uapi/asm/ioctls.h             |  3 +
 arch/sparc/include/uapi/asm/ioctls.h          |  3 +
 arch/xtensa/include/uapi/asm/ioctls.h         |  3 +
 drivers/tty/serial/8250/8250_core.c           |  2 +
 drivers/tty/serial/serial_core.c              | 62 ++++++++++++++++++-
 drivers/tty/tty_io.c                          |  2 +
 include/linux/serial_core.h                   |  6 ++
 include/uapi/asm-generic/ioctls.h             |  3 +
 include/uapi/linux/serial.h                   |  8 +++
 14 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/serial/serial-rs485.rst b/Documentation/driver-api/serial/serial-rs485.rst
index 6bc824f948f9..2f45f007fa5b 100644
--- a/Documentation/driver-api/serial/serial-rs485.rst
+++ b/Documentation/driver-api/serial/serial-rs485.rst
@@ -95,7 +95,28 @@ RS485 Serial Communications
 		/* Error handling. See errno. */
 	}
 
-5. References
+5. Multipoint Addressing
+========================
+
+   The Linux kernel provides serial_addr structure to handle addressing within
+   multipoint serial communications line such as RS485. 9th bit addressiong mode
+   is enabled by adding ADDRB flag in termios c_cflag.
+
+   Serial core calls device specific set/get_addr in response to TIOCSADDR and
+   TIOCGADDR ioctls with a pointer to serial_addr. Destination and receive
+   address can be specified using serial_addr flags field. Receive address may
+   also be cleared using flags. Once an address is set, the communication
+   can occur only with the particular device and other peers are filtered out.
+   It is left up to the receiver side to enforce the filtering.
+
+   Address flags:
+	- SER_ADDR_RECV: Receive (filter) address.
+	- SER_ADDR_RECV_CLEAR: Clear receive filter (only for TIOCSADDR).
+	- SER_ADDR_DEST: Destination address.
+
+   Note: not all devices supporting RS485 support multipoint addressing.
+
+6. References
 =============
 
  [1]	include/uapi/linux/serial.h
diff --git a/arch/alpha/include/uapi/asm/ioctls.h b/arch/alpha/include/uapi/asm/ioctls.h
index 971311605288..500cab3e1d6b 100644
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -125,4 +125,7 @@
 #define TIOCMIWAIT	0x545C	/* wait for a change on serial input line(s) */
 #define TIOCGICOUNT	0x545D	/* read serial port inline interrupt counts */
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 #endif /* _ASM_ALPHA_IOCTLS_H */
diff --git a/arch/mips/include/uapi/asm/ioctls.h b/arch/mips/include/uapi/asm/ioctls.h
index 16aa8a766aec..3859dc46857e 100644
--- a/arch/mips/include/uapi/asm/ioctls.h
+++ b/arch/mips/include/uapi/asm/ioctls.h
@@ -96,6 +96,9 @@
 #define TIOCGISO7816	_IOR('T', 0x42, struct serial_iso7816)
 #define TIOCSISO7816	_IOWR('T', 0x43, struct serial_iso7816)
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 /* I hope the range from 0x5480 on is free ... */
 #define TIOCSCTTY	0x5480		/* become controlling tty */
 #define TIOCGSOFTCAR	0x5481
diff --git a/arch/parisc/include/uapi/asm/ioctls.h b/arch/parisc/include/uapi/asm/ioctls.h
index 82d1148c6379..62337743db64 100644
--- a/arch/parisc/include/uapi/asm/ioctls.h
+++ b/arch/parisc/include/uapi/asm/ioctls.h
@@ -86,6 +86,9 @@
 #define TIOCSTOP	0x5462
 #define TIOCSLTC	0x5462
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 /* Used for packet mode */
 #define TIOCPKT_DATA		 0
 #define TIOCPKT_FLUSHREAD	 1
diff --git a/arch/powerpc/include/uapi/asm/ioctls.h b/arch/powerpc/include/uapi/asm/ioctls.h
index 2c145da3b774..84fd69ac366a 100644
--- a/arch/powerpc/include/uapi/asm/ioctls.h
+++ b/arch/powerpc/include/uapi/asm/ioctls.h
@@ -120,4 +120,7 @@
 #define TIOCMIWAIT	0x545C	/* wait for a change on serial input line(s) */
 #define TIOCGICOUNT	0x545D	/* read serial port inline interrupt counts */
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 #endif	/* _ASM_POWERPC_IOCTLS_H */
diff --git a/arch/sh/include/uapi/asm/ioctls.h b/arch/sh/include/uapi/asm/ioctls.h
index 11866d4f60e1..f82966b7dba2 100644
--- a/arch/sh/include/uapi/asm/ioctls.h
+++ b/arch/sh/include/uapi/asm/ioctls.h
@@ -113,4 +113,7 @@
 #define TIOCMIWAIT	_IO('T', 92) /* 0x545C */	/* wait for a change on serial input line(s) */
 #define TIOCGICOUNT	0x545D	/* read serial port inline interrupt counts */
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 #endif /* __ASM_SH_IOCTLS_H */
diff --git a/arch/sparc/include/uapi/asm/ioctls.h b/arch/sparc/include/uapi/asm/ioctls.h
index 7fd2f5873c9e..e44624c67c79 100644
--- a/arch/sparc/include/uapi/asm/ioctls.h
+++ b/arch/sparc/include/uapi/asm/ioctls.h
@@ -125,6 +125,9 @@
 #define TIOCMIWAIT	0x545C /* Wait for change on serial input line(s) */
 #define TIOCGICOUNT	0x545D /* Read serial port inline interrupt counts */
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 /* Kernel definitions */
 
 /* Used for packet mode */
diff --git a/arch/xtensa/include/uapi/asm/ioctls.h b/arch/xtensa/include/uapi/asm/ioctls.h
index 6d4a87296c95..759ca9377f2a 100644
--- a/arch/xtensa/include/uapi/asm/ioctls.h
+++ b/arch/xtensa/include/uapi/asm/ioctls.h
@@ -127,4 +127,7 @@
 #define TIOCMIWAIT	_IO('T', 92) /* wait for a change on serial input line(s) */
 #define TIOCGICOUNT	0x545D	/* read serial port inline interrupt counts */
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 #endif /* _XTENSA_IOCTLS_H */
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 01d30f6ed8fb..f67bc3b76f65 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1008,6 +1008,8 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 		uart->port.rs485	= up->port.rs485;
 		uart->rs485_start_tx	= up->rs485_start_tx;
 		uart->rs485_stop_tx	= up->rs485_stop_tx;
+		uart->port.set_addr	= up->port.set_addr;
+		uart->port.get_addr	= up->port.get_addr;
 		uart->dma		= up->dma;
 
 		/* Take tx_loadsz from fifosize if it wasn't set separately */
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index de198c2acefe..2cd129c78ef6 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1350,6 +1350,56 @@ static int uart_set_iso7816_config(struct uart_port *port,
 	return 0;
 }
 
+static int uart_set_addr(struct uart_port *port,
+			 struct serial_addr __user *serial_addr_user)
+{
+	struct serial_addr addr;
+	unsigned long flags;
+	int ret;
+
+	if (!port->set_addr)
+		return -ENOTTY;
+
+	if (copy_from_user(&addr, serial_addr_user, sizeof(*serial_addr_user)))
+		return -EFAULT;
+
+	spin_lock_irqsave(&port->lock, flags);
+	ret = port->set_addr(port, &addr);
+	spin_unlock_irqrestore(&port->lock, flags);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(serial_addr_user, &addr, sizeof(addr)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int uart_get_addr(struct uart_port *port,
+			 struct serial_addr __user *serial_addr_user)
+{
+	struct serial_addr addr;
+	unsigned long flags;
+	int ret;
+
+	if (!port->get_addr)
+		return -ENOTTY;
+
+	if (copy_from_user(&addr, serial_addr_user, sizeof(*serial_addr_user)))
+		return -EFAULT;
+
+	spin_lock_irqsave(&port->lock, flags);
+	ret = port->get_addr(port, &addr);
+	spin_unlock_irqrestore(&port->lock, flags);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(serial_addr_user, &addr, sizeof(addr)))
+		return -EFAULT;
+
+	return 0;
+}
+
 /*
  * Called via sys_ioctl.  We can use spin_lock_irq() here.
  */
@@ -1427,6 +1477,15 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 	case TIOCGISO7816:
 		ret = uart_get_iso7816_config(state->uart_port, uarg);
 		break;
+
+	case TIOCSADDR:
+		ret = uart_set_addr(uport, uarg);
+		break;
+
+	case TIOCGADDR:
+		ret = uart_get_addr(uport, uarg);
+		break;
+
 	default:
 		if (uport->ops->ioctl)
 			ret = uport->ops->ioctl(uport, cmd, arg);
@@ -1493,7 +1552,8 @@ static void uart_set_termios(struct tty_struct *tty,
 		goto out;
 	}
 
-	tty->termios.c_cflag &= ~ADDRB;
+	if (!uport->set_addr)
+		tty->termios.c_cflag &= ~ADDRB;
 
 	uart_change_speed(tty, state, old_termios);
 	/* reload cflag from termios; port driver may have overridden flags */
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7e8b3bd59c7b..93d8609e88aa 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2885,6 +2885,8 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 	case TIOCSERGETLSR:
 	case TIOCGRS485:
 	case TIOCSRS485:
+	case TIOCSADDR:
+	case TIOCGADDR:
 #ifdef TIOCGETP
 	case TIOCGETP:
 	case TIOCSETP:
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 504d365e2803..a2efd3fe2635 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -135,6 +135,12 @@ struct uart_port {
 						struct serial_rs485 *rs485);
 	int			(*iso7816_config)(struct uart_port *,
 						  struct serial_iso7816 *iso7816);
+
+	int			(*set_addr)(struct uart_port *p,
+					    struct serial_addr *addr);
+	int			(*get_addr)(struct uart_port *p,
+					    struct serial_addr *addr);
+
 	unsigned int		irq;			/* irq number */
 	unsigned long		irqflags;		/* irq flags  */
 	unsigned int		uartclk;		/* base uart clock */
diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
index cdc9f4ca8c27..689743366091 100644
--- a/include/uapi/asm-generic/ioctls.h
+++ b/include/uapi/asm-generic/ioctls.h
@@ -106,6 +106,9 @@
 # define FIOQSIZE	0x5460
 #endif
 
+#define TIOCSADDR	_IOWR('T', 0x63, struct serial_addr)
+#define TIOCGADDR	_IOWR('T', 0x64, struct serial_addr)
+
 /* Used for packet mode */
 #define TIOCPKT_DATA		 0
 #define TIOCPKT_FLUSHREAD	 1
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index fa6b16e5fdd8..8cb785ea7087 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -149,4 +149,12 @@ struct serial_iso7816 {
 	__u32	reserved[5];
 };
 
+struct serial_addr {
+	__u32	flags;
+#define SER_ADDR_RECV			(1 << 0)
+#define SER_ADDR_RECV_CLEAR		(1 << 1)
+#define SER_ADDR_DEST			(1 << 2)
+	__u32	addr;
+};
+
 #endif /* _UAPI_LINUX_SERIAL_H */
-- 
2.30.2

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Vicente Bergas]

Hello Ilpo,

i have tested your v3 patch on v3 hardware, that is, using the
emulated em485 because of lack of HW support. It is not working
due to three issues.

1.- rs485_stop_tx is never called because there are no interrupts.
I worked around this by disabling DMA:

--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
 		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
 		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
- 		up->dma = &data->data.dma;
+		up->dma = 0; // Proof of concept, not to be merged!

2.- Although "linux,rs485-enabled-at-boot-time" is set in the DTS,
the RTS/DriverEnable line is asserted all the time the /dev/ttyS1
device file is closed.
As soon as the device file is openned, the RTS line is deasserted.
Then it works as expected: it is asserted only during transmissions.
When the device file is closed again, the RTS line goes back to the
asserted level and stays there.
When the rs485 mode is enabled, it is expected that the RTS line be
deasserted by default.

3.- The RTS line is asserted a few microseconds earlier than the
start bit, that is acceptable, but then it deasserts one whole bit
time before the last stop bit.
So, the last stop bit of the last byte of a message is not sent
because the driver is disabled.
This has been tested with the port configured at 19200e1, that is,
the bit time is 52 us.
I worked around this by adding "rs485-rts-delay = <0 52>;" in the
DTS. This leads to the following feature (not an issue):

On Mon, 11 Apr 2022 11:33:12 +0300, Ilpo Järvinen wrote:
> Set delay_rts_before_send and delay_rts_after_send to zero for now.
> The granularity of that ABI is too coarse to be useful.

Indeed the time unit of this parameter is milliseconds, as stated in
Documentation/devicetree/bindings/serial/rs485.yaml
Which in the general case is more than ten bit times.

But it is being interpreted as microseconds here:

On Mon, 11 Apr 2022 11:33:11 +0300, Ilpo Järvinen wrote:
> [PATCH v3 02/12] serial: 8250: Handle UART without interrupt on TEMT
>+	stop_delay += (u64)p->port.rs485.delay_rts_after_send * NSEC_PER_USEC;

So, this way it has a useful granularity to be used in
"rs485-rts-delay = <0 52>;" but is not compliant with the spec.

Regards,
  Vicente.

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Lukas Wunner]

On Thu, Apr 21, 2022 at 05:36:26PM +0200, Vicente Bergas wrote:
> 2.- Although "linux,rs485-enabled-at-boot-time" is set in the DTS,
> the RTS/DriverEnable line is asserted all the time the /dev/ttyS1
> device file is closed.
> As soon as the device file is openned, the RTS line is deasserted.
> Then it works as expected: it is asserted only during transmissions.
> When the device file is closed again, the RTS line goes back to the
> asserted level and stays there.
> When the rs485 mode is enabled, it is expected that the RTS line be
> deasserted by default.

That's probably the same issue as reported here:

https://lore.kernel.org/linux-serial/20220329085050.311408-1-matthias.schiffer@ew.tq-group.com/

Basically, some drivers have historically used inverse RTS polarity
and others haven't.  I'm still busy sorting out which drivers use
which.  Sorry, this is quite complex and takes some time.

Have you configured "rs485-rts-active-low" in the device tree?
Which kernel version are you using exactly?

Thanks,

Lukas

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Vicente Bergas]

Hi Lukas,

On Thu, Apr 21, 2022 at 9:38 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Apr 21, 2022 at 05:36:26PM +0200, Vicente Bergas wrote:
> > 2.- Although "linux,rs485-enabled-at-boot-time" is set in the DTS,
> > the RTS/DriverEnable line is asserted all the time the /dev/ttyS1
> > device file is closed.
> > As soon as the device file is openned, the RTS line is deasserted.
> > Then it works as expected: it is asserted only during transmissions.
> > When the device file is closed again, the RTS line goes back to the
> > asserted level and stays there.
> > When the rs485 mode is enabled, it is expected that the RTS line be
> > deasserted by default.
>
> That's probably the same issue as reported here:
>
> https://lore.kernel.org/linux-serial/20220329085050.311408-1-matthias.schiffer@ew.tq-group.com/
>
> Basically, some drivers have historically used inverse RTS polarity
> and others haven't.  I'm still busy sorting out which drivers use
> which.  Sorry, this is quite complex and takes some time.
>
> Have you configured "rs485-rts-active-low" in the device tree?

The UART configuration is the default from rk3328.dtsi plus:
&uart1 {
  linux,rs485-enabled-at-boot-time;
  pinctrl-0 = <&uart1_xfer &uart1_cts &uart1_rts_pin>;
  rs485-rts-delay = <0 52>;
  rs485-rx-during-tx;
  rts-gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
  status = "okay";
};

So, it is asserted high and deasserted low.

> Which kernel version are you using exactly?

5.17.3 and 5.17.4
Also, i am about to test 5.18-rc3

> Thanks,
>
> Lukas

Regards,
  Vicente.

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Ilpo Järvinen]

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

On Thu, 21 Apr 2022, Vicente Bergas wrote:

> i have tested your v3 patch on v3 hardware, that is, using the
> emulated em485 because of lack of HW support. It is not working
> due to three issues.

Thanks for testing!

> 1.- rs485_stop_tx is never called because there are no interrupts.
> I worked around this by disabling DMA:
> 
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
>  		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
>  		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> - 		up->dma = &data->data.dma;
> +		up->dma = 0; // Proof of concept, not to be merged!

I'll need to look into this.

> 2.- Although "linux,rs485-enabled-at-boot-time" is set in the DTS,
> the RTS/DriverEnable line is asserted all the time the /dev/ttyS1
> device file is closed.
> As soon as the device file is openned, the RTS line is deasserted.
> Then it works as expected: it is asserted only during transmissions.
> When the device file is closed again, the RTS line goes back to the
> asserted level and stays there.
> When the rs485 mode is enabled, it is expected that the RTS line be
> deasserted by default.

Managing RTS is a mess in 8250 driver as has recently being noted. It will 
hopefully get sorted out eventually.

> 3.- The RTS line is asserted a few microseconds earlier than the
> start bit, that is acceptable, but then it deasserts one whole bit
> time before the last stop bit.
> So, the last stop bit of the last byte of a message is not sent
> because the driver is disabled.
> This has been tested with the port configured at 19200e1, that is,
> the bit time is 52 us.
> I worked around this by adding "rs485-rts-delay = <0 52>;" in the
> DTS. This leads to the following feature (not an issue):
> 
> On Mon, 11 Apr 2022 11:33:12 +0300, Ilpo Järvinen wrote:
> > Set delay_rts_before_send and delay_rts_after_send to zero for now.
> > The granularity of that ABI is too coarse to be useful.
> 
> Indeed the time unit of this parameter is milliseconds, as stated in
> Documentation/devicetree/bindings/serial/rs485.yaml
> Which in the general case is more than ten bit times.
> 
> But it is being interpreted as microseconds here:
> 
> On Mon, 11 Apr 2022 11:33:11 +0300, Ilpo Järvinen wrote:
> > [PATCH v3 02/12] serial: 8250: Handle UART without interrupt on TEMT
> >+	stop_delay += (u64)p->port.rs485.delay_rts_after_send * NSEC_PER_USEC;
> 
> So, this way it has a useful granularity to be used in
> "rs485-rts-delay = <0 52>;" but is not compliant with the spec.

It seems I just got confused here with all these different time units. My 
intention was to use NSEC_PER_MSEC here to match the spec although it's 
not that useful granularity. I'll change that for the next version.
Lukas was planning of making it much finer granularity with nsecs (leaving 
the small values for msecs for compat purposes) which would hopefully 
resolve this granularity challenge.

About the actual issue of too early deassert. It kind of sounds like the 
stop tx timer was not waiting long enough. It could be that THRE is 
asserted sooner than I expected (maybe HW does FIFO->shift register
during the transmission of the stop bit of the prev char asserting THRE
approx one bit too early).

Perhaps this patch would help to combat the problem (roughly estimating
worst-case one bit time here with that /7):

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 35fbaa53bc2f..f944c639db82 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1551,7 +1551,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		if (!(lsr & UART_LSR_TEMT)) {
 			if (!(p->capabilities & UART_CAP_NOTEMT))
 				return;
-			stop_delay = p->port.frame_time;
+			stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
 		}
 
 		__stop_tx_rs485(p, stop_delay);

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Ilpo Järvinen]

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

On Fri, 22 Apr 2022, Ilpo Järvinen wrote:

> On Thu, 21 Apr 2022, Vicente Bergas wrote:
> 
> > i have tested your v3 patch on v3 hardware, that is, using the
> > emulated em485 because of lack of HW support. It is not working
> > due to three issues.
> 
> Thanks for testing!
> 
> > 1.- rs485_stop_tx is never called because there are no interrupts.
> > I worked around this by disabling DMA:
> > 
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
> >  		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
> >  		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> > - 		up->dma = &data->data.dma;
> > +		up->dma = 0; // Proof of concept, not to be merged!
> 
> I'll need to look into this.

8250 DMA tx complete path lacks calls to normal 8250 stop handling and
I think it probably also assumes too much from dmaengine's completion 
callback. ...It also seems bit early to call serial8250_rpm_put_tx from 
there(?).

This patch allowed em485_start/stop_tx to be called in my tests:


[PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA

Currently, DMA complete handling in 8250 driver does not use THRE
to detect true completion of the tx and also doesn't call __stop_tx.
This leads to problems with em485 that needs to handle RTS timing.

Instead of handling tx stop within 8250 dma code, enable THRE when
tx data runs out and tweak serial8250_handle_irq to call
__stop_tx when uart is using DMA.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c  | 4 ++--
 drivers/tty/serial/8250/8250_port.c | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 890fa7ddaa7f..e84db0abf365 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -29,12 +29,13 @@ static void __dma_tx_complete(void *param)
 	xmit->tail += dma->tx_size;
 	xmit->tail &= UART_XMIT_SIZE - 1;
 	p->port.icount.tx += dma->tx_size;
+	dma->tx_size = 0;
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
 	ret = serial8250_tx_dma(p);
-	if (ret)
+	if (ret || !dma->tx_size)
 		serial8250_set_THRI(p);
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
@@ -71,7 +72,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 
 	if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
 		/* We have been called from __dma_tx_complete() */
-		serial8250_rpm_put_tx(p);
 		return 0;
 	}
 
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index b3289ef117d1..72f144449ee1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1960,9 +1960,12 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 			status = serial8250_rx_chars(up, status);
 	}
 	serial8250_modem_status(up);
-	if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE) &&
-		(up->ier & UART_IER_THRI))
-		serial8250_tx_chars(up);
+	if ((status & UART_LSR_THRE) && (up->ier & UART_IER_THRI)) {
+		if (!up->dma || up->dma->tx_err)
+			serial8250_tx_chars(up);
+		else
+			__stop_tx(up);
+	}
 
 	uart_unlock_and_check_sysrq_irqrestore(port, flags);
 
-- 
2.35.1

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Vicente Bergas]

Hi Ilpo, thank you for your quick reply with a solution!

On Fri, Apr 22, 2022 at 3:07 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 22 Apr 2022, Ilpo Järvinen wrote:
>
> > On Thu, 21 Apr 2022, Vicente Bergas wrote:
> >
> > > 1.- rs485_stop_tx is never called because there are no interrupts.
> > > I worked around this by disabling DMA:
> >
> > I'll need to look into this.
>
> 8250 DMA tx complete path lacks calls to normal 8250 stop handling and
> I think it probably also assumes too much from dmaengine's completion
> callback. ...It also seems bit early to call serial8250_rpm_put_tx from
> there(?).
>
> This patch allowed em485_start/stop_tx to be called in my tests:
> [PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA

I confirm that this patch fixes the issue when DMA is enabled.

I also confirm that your other patch
> +                       stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
fixes the issue with RTS/DriverEnable deassertion time.
I've tested it again at 19200e1 and now RTS is deasserted
approximately at the same time as the end of the stop bit.
Please, note the "e" for even parity bit, that extra bit after the
data byte might have an impact on this timings.

Regards,
  Vicente.

On Fri, Apr 22, 2022 at 3:07 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 22 Apr 2022, Ilpo Järvinen wrote:
>
> > On Thu, 21 Apr 2022, Vicente Bergas wrote:
> >
> > > i have tested your v3 patch on v3 hardware, that is, using the
> > > emulated em485 because of lack of HW support. It is not working
> > > due to three issues.
> >
> > Thanks for testing!
> >
> > > 1.- rs485_stop_tx is never called because there are no interrupts.
> > > I worked around this by disabling DMA:
> > >
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
> > >             data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
> > >             data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> > > -           up->dma = &data->data.dma;
> > > +           up->dma = 0; // Proof of concept, not to be merged!
> >
> > I'll need to look into this.
>
> 8250 DMA tx complete path lacks calls to normal 8250 stop handling and
> I think it probably also assumes too much from dmaengine's completion
> callback. ...It also seems bit early to call serial8250_rpm_put_tx from
> there(?).
>
> This patch allowed em485_start/stop_tx to be called in my tests:
>
>
> [PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA
>
> Currently, DMA complete handling in 8250 driver does not use THRE
> to detect true completion of the tx and also doesn't call __stop_tx.
> This leads to problems with em485 that needs to handle RTS timing.
>
> Instead of handling tx stop within 8250 dma code, enable THRE when
> tx data runs out and tweak serial8250_handle_irq to call
> __stop_tx when uart is using DMA.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dma.c  | 4 ++--
>  drivers/tty/serial/8250/8250_port.c | 9 ++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 890fa7ddaa7f..e84db0abf365 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -29,12 +29,13 @@ static void __dma_tx_complete(void *param)
>         xmit->tail += dma->tx_size;
>         xmit->tail &= UART_XMIT_SIZE - 1;
>         p->port.icount.tx += dma->tx_size;
> +       dma->tx_size = 0;
>
>         if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>                 uart_write_wakeup(&p->port);
>
>         ret = serial8250_tx_dma(p);
> -       if (ret)
> +       if (ret || !dma->tx_size)
>                 serial8250_set_THRI(p);
>
>         spin_unlock_irqrestore(&p->port.lock, flags);
> @@ -71,7 +72,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>
>         if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
>                 /* We have been called from __dma_tx_complete() */
> -               serial8250_rpm_put_tx(p);
>                 return 0;
>         }
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index b3289ef117d1..72f144449ee1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1960,9 +1960,12 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
>                         status = serial8250_rx_chars(up, status);
>         }
>         serial8250_modem_status(up);
> -       if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE) &&
> -               (up->ier & UART_IER_THRI))
> -               serial8250_tx_chars(up);
> +       if ((status & UART_LSR_THRE) && (up->ier & UART_IER_THRI)) {
> +               if (!up->dma || up->dma->tx_err)
> +                       serial8250_tx_chars(up);
> +               else
> +                       __stop_tx(up);
> +       }
>
>         uart_unlock_and_check_sysrq_irqrestore(port, flags);
>
> --
> 2.35.1

Re: [PATCH v3 00/12] Add RS485 support to DW UART

[Ilpo Järvinen]

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

On Sun, 24 Apr 2022, Vicente Bergas wrote:

> Hi Ilpo, thank you for your quick reply with a solution!
> 
> On Fri, Apr 22, 2022 at 3:07 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Fri, 22 Apr 2022, Ilpo Järvinen wrote:
> >
> > > On Thu, 21 Apr 2022, Vicente Bergas wrote:
> > >
> > > > 1.- rs485_stop_tx is never called because there are no interrupts.
> > > > I worked around this by disabling DMA:
> > >
> > > I'll need to look into this.
> >
> > 8250 DMA tx complete path lacks calls to normal 8250 stop handling and
> > I think it probably also assumes too much from dmaengine's completion
> > callback. ...It also seems bit early to call serial8250_rpm_put_tx from
> > there(?).
> >
> > This patch allowed em485_start/stop_tx to be called in my tests:
> > [PATCH 1/1] serial: 8250: use THRE & __stop_tx also with DMA
> 
> I confirm that this patch fixes the issue when DMA is enabled.
> 
> I also confirm that your other patch
> > +                       stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
> fixes the issue with RTS/DriverEnable deassertion time.
> I've tested it again at 19200e1 and now RTS is deasserted
> approximately at the same time as the end of the stop bit.
> Please, note the "e" for even parity bit, that extra bit after the
> data byte might have an impact on this timings.

Great, thanks again for testing.

I don't think that parity "e" makes the calculations incorrect as parity 
bit should be taken into account in tty_get_frame_size(). My guess is it's 
more about whether THRE might get asserted already during the stop bit or 
only after the stop bit has been fully sent.


-- 
 i.