2022-06-24 20:55:38

by Ilpo Järvinen

[permalink] [raw]
Subject: [PATCH v9 0/6] Add RS485 9th bit addressing mode support to DW UART

This patchset adds RS-485 9th bit addressing mode support to the DW
UART driver and the necessary serial core bits to handle it. The
addressing mode is configured through ->rs485_config() as was requested
during the review of the earlier versions. The line configuration
related ADDRB is still kept in ktermios->c_cflag to be able to take
account the extra addressing bit while calculating timing, etc. but it
is set/cleared by ->rs485_config().

Cc: Arnd Bergmann <[email protected]>
Cc: [email protected]
Cc: [email protected]

v8 -> v9:
- Tweak comments (struct fields info & one callback style correction)
- Add struct layout checks as static_asserts

v7 -> v8:
- Use anonymous union/struct in serial_rs485 to create "v2" of it
- Remove a stray newline change
- Reorder local var declarations
- Put ktermios param before serial_rs485 for rs485_config

v6 -> v7:
- Fixed typos in documentation & comment
- Changes lsr typing from unsigned int to u16

v5 -> v6:
- Reorder remaining patches
- LSR changes are simpler due to helper added by LSR fix series
- Depend on rs485_struct sanitization on catching much of invalid config
- In order to be able to alter ADDRB in termios .c_cflag within
.rs485_config(), take termios_rwsem and pass ktermios to it.
- Moved addressing mode setup entirely into .rs485_config()
- Use ndelay() instead of udelay() (uart_port->frame_time is in nsecs)


Ilpo Järvinen (6):
serial: 8250: make saved LSR larger
serial: 8250: create lsr_save_mask
serial: 8250_lpss: Use 32-bit reads
serial: take termios_rwsem for ->rs485_config() & pass termios as
param
serial: Support for RS-485 multipoint addresses
serial: 8250_dwlib: Support for 9th bit multipoint addressing

Documentation/driver-api/serial/driver.rst | 2 +
.../driver-api/serial/serial-rs485.rst | 26 ++++-
drivers/tty/serial/8250/8250.h | 9 +-
drivers/tty/serial/8250/8250_core.c | 4 +
drivers/tty/serial/8250/8250_dw.c | 2 +-
drivers/tty/serial/8250/8250_dwlib.c | 105 +++++++++++++++++-
drivers/tty/serial/8250/8250_exar.c | 11 +-
drivers/tty/serial/8250/8250_fintek.c | 2 +-
drivers/tty/serial/8250/8250_fsl.c | 2 +-
drivers/tty/serial/8250/8250_ingenic.c | 2 +-
drivers/tty/serial/8250/8250_lpc18xx.c | 2 +-
drivers/tty/serial/8250/8250_lpss.c | 2 +-
drivers/tty/serial/8250/8250_omap.c | 7 +-
drivers/tty/serial/8250/8250_pci.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 20 ++--
drivers/tty/serial/amba-pl011.c | 2 +-
drivers/tty/serial/ar933x_uart.c | 2 +-
drivers/tty/serial/atmel_serial.c | 2 +-
drivers/tty/serial/fsl_lpuart.c | 4 +-
drivers/tty/serial/imx.c | 2 +-
drivers/tty/serial/max310x.c | 2 +-
drivers/tty/serial/mcf.c | 3 +-
drivers/tty/serial/omap-serial.c | 3 +-
drivers/tty/serial/sc16is7xx.c | 2 +-
drivers/tty/serial/serial_core.c | 36 +++++-
drivers/tty/serial/stm32-usart.c | 2 +-
drivers/tty/tty_ioctl.c | 4 +
include/linux/serial_8250.h | 7 +-
include/linux/serial_core.h | 1 +
include/uapi/asm-generic/termbits-common.h | 1 +
include/uapi/linux/serial.h | 20 +++-
31 files changed, 238 insertions(+), 53 deletions(-)

--
2.30.2


2022-06-24 20:56:45

by Ilpo Järvinen

[permalink] [raw]
Subject: [PATCH v9 5/6] serial: Support for RS-485 multipoint addresses

Add support for RS-485 multipoint addressing using 9th bit [*]. The
addressing mode is configured through ->rs485_config().

ADDRB in termios indicates 9th bit addressing mode is enabled. In this
mode, 9th bit is used to indicate an address (byte) within the
communication line. ADDRB can only be enabled/disabled through
->rs485_config() that is also responsible for setting the destination and
receiver (filter) addresses.

Add traps to detect unwanted changes to struct serial_rs485 layout using
static_assert().

[*] 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.

Signed-off-by: Ilpo Järvinen <[email protected]>

---
Cc: Arnd Bergmann <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Documentation/driver-api/serial/driver.rst | 2 ++
.../driver-api/serial/serial-rs485.rst | 26 ++++++++++++++++++-
drivers/tty/serial/serial_core.c | 22 +++++++++++++++-
drivers/tty/tty_ioctl.c | 4 +++
include/uapi/asm-generic/termbits-common.h | 1 +
include/uapi/linux/serial.h | 20 ++++++++++++--
6 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
index 7ef83fd3917b..35fb3866bb3d 100644
--- a/Documentation/driver-api/serial/driver.rst
+++ b/Documentation/driver-api/serial/driver.rst
@@ -261,6 +261,8 @@ hardware.
- parity enable
PARODD
- odd parity (when PARENB is in force)
+ ADDRB
+ - address bit (changed through .rs485_config()).
CREAD
- enable reception of characters (if not set,
still receive characters from the port, but
diff --git a/Documentation/driver-api/serial/serial-rs485.rst b/Documentation/driver-api/serial/serial-rs485.rst
index 00b5d333acba..6ebad75c74ed 100644
--- a/Documentation/driver-api/serial/serial-rs485.rst
+++ b/Documentation/driver-api/serial/serial-rs485.rst
@@ -99,7 +99,31 @@ RS485 Serial Communications
/* Error handling. See errno. */
}

-5. References
+5. Multipoint Addressing
+========================
+
+ The Linux kernel provides addressing mode for multipoint RS-485 serial
+ communications line. The addressing mode is enabled with SER_RS485_ADDRB
+ flag in serial_rs485. Struct serial_rs485 has two additional flags and
+ fields for enabling receive and destination addresses.
+
+ Address mode flags:
+ - SER_RS485_ADDRB: Enabled addressing mode (sets also ADDRB in termios).
+ - SER_RS485_ADDR_RECV: Receive (filter) address enabled.
+ - SER_RS485_ADDR_DEST: Set destination address.
+
+ Address fields (enabled with corresponding SER_RS485_ADDR_* flag):
+ - addr_recv: Receive address.
+ - addr_dest: Destination address.
+
+ Once a receive 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. Receive address will be cleared
+ if SER_RS485_ADDR_RECV is not set.
+
+ Note: not all devices supporting RS485 support multipoint addressing.
+
+6. References
=============

[1] include/uapi/linux/serial.h
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 44c3785445e3..0aecaf753dea 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1294,6 +1294,17 @@ static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *r
if (flags & ~port->rs485_supported->flags)
return -EINVAL;

+ /* Asking for address w/o addressing mode? */
+ if (!(rs485->flags & SER_RS485_ADDRB) &&
+ (rs485->flags & (SER_RS485_ADDR_RECV|SER_RS485_ADDR_DEST)))
+ return -EINVAL;
+
+ /* Address given but not enabled? */
+ if (!(rs485->flags & SER_RS485_ADDR_RECV) && rs485->addr_recv)
+ return -EINVAL;
+ if (!(rs485->flags & SER_RS485_ADDR_DEST) && rs485->addr_dest)
+ return -EINVAL;
+
return 0;
}

@@ -1349,7 +1360,8 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
rs485->flags &= supported_flags;

/* Return clean padding area to userspace */
- memset(rs485->padding, 0, sizeof(rs485->padding));
+ memset(rs485->padding0, 0, sizeof(rs485->padding0));
+ memset(rs485->padding1, 0, sizeof(rs485->padding1));
}

int uart_rs485_config(struct uart_port *port)
@@ -3404,5 +3416,13 @@ int uart_get_rs485_mode(struct uart_port *port)
}
EXPORT_SYMBOL_GPL(uart_get_rs485_mode);

+/* Compile-time assertions for serial_rs485 layout */
+static_assert(offsetof(struct serial_rs485, padding) ==
+ (offsetof(struct serial_rs485, delay_rts_after_send) + sizeof(__u32)));
+static_assert(offsetof(struct serial_rs485, padding1) ==
+ offsetof(struct serial_rs485, padding[1]));
+static_assert((offsetof(struct serial_rs485, padding[4]) + sizeof(__u32)) ==
+ sizeof(struct serial_rs485));
+
MODULE_DESCRIPTION("Serial driver core");
MODULE_LICENSE("GPL");
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index adae687f654b..2a76b330e108 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;
}
@@ -353,6 +355,8 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
old_termios = tty->termios;
tty->termios = *new_termios;
unset_locked_termios(tty, &old_termios);
+ /* Reset any ADDRB changes, ADDRB is changed through ->rs485_config() */
+ tty->termios.c_cflag ^= (tty->termios.c_cflag ^ old_termios.c_cflag) & ADDRB;

if (tty->ops->set_termios)
tty->ops->set_termios(tty, &old_termios);
diff --git a/include/uapi/asm-generic/termbits-common.h b/include/uapi/asm-generic/termbits-common.h
index 4d084fe8def5..4a6a79f28b21 100644
--- a/include/uapi/asm-generic/termbits-common.h
+++ b/include/uapi/asm-generic/termbits-common.h
@@ -46,6 +46,7 @@ typedef unsigned int speed_t;
#define EXTA B19200
#define EXTB B38400

+#define ADDRB 0x20000000 /* address bit */
#define CMSPAR 0x40000000 /* mark or space (stick) parity */
#define CRTSCTS 0x80000000 /* flow control */

diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index fa6b16e5fdd8..cea06924b295 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -126,10 +126,26 @@ struct serial_rs485 {
#define SER_RS485_TERMINATE_BUS (1 << 5) /* Enable bus
termination
(if supported) */
+
+/* RS-485 addressing mode */
+#define SER_RS485_ADDRB (1 << 6) /* Enable addressing mode */
+#define SER_RS485_ADDR_RECV (1 << 7) /* Receive address filter */
+#define SER_RS485_ADDR_DEST (1 << 8) /* Destination address */
+
__u32 delay_rts_before_send; /* Delay before send (milliseconds) */
__u32 delay_rts_after_send; /* Delay after send (milliseconds) */
- __u32 padding[5]; /* Memory is cheap, new structs
- are a royal PITA .. */
+
+ /* The fields below are defined by flags */
+ union {
+ __u32 padding[5]; /* Memory is cheap, new structs are a pain */
+
+ struct {
+ __u8 addr_recv;
+ __u8 addr_dest;
+ __u8 padding0[2];
+ __u32 padding1[4];
+ };
+ };
};

/*
--
2.30.2

2022-06-24 20:57:33

by Ilpo Järvinen

[permalink] [raw]
Subject: [PATCH v9 3/6] serial: 8250_lpss: Use 32-bit reads

Use 32-bit reads in order to not lose higher bits of DW UART regs. This
change does not fix any known issue as the high bits are not used for
anything related to 8250 driver (dw8250_readl_ext and dw8250_writel_ext
used within the dwlib are already doing
readl/writel/ioread32be/iowrite32be anyway).

This change is necessary to enables 9th bit address mode. DW UART
reports address frames with BIT(8) of LSR.

Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_lpss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 0f5af061e0b4..4ba43bef9933 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -330,7 +330,7 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
uart.port.irq = pci_irq_vector(pdev, 0);
uart.port.private_data = &lpss->data;
uart.port.type = PORT_16550A;
- uart.port.iotype = UPIO_MEM;
+ uart.port.iotype = UPIO_MEM32;
uart.port.regshift = 2;
uart.port.uartclk = lpss->board->base_baud * 16;
uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE;
--
2.30.2

2022-06-24 21:05:08

by Ilpo Järvinen

[permalink] [raw]
Subject: [PATCH v9 6/6] serial: 8250_dwlib: Support for 9th bit multipoint addressing

Add 9th bit multipoint addressing mode for DW UART. 9th bit addressing
can be used only when HW RS485 is available.

Updating RAR (receive address register) is bit tricky because busy
indication is not be available when DW UART is strictly 16550
compatible, which is the case with the hardware I was testing with. RAR
should not be updated while receive is in progress which is now
achieved by deasserting RE and waiting for one frame (in case rx would
be in progress, the driver seems to have no way of knowing it w/o busy
indication). Because of this complexity, it's better to avoid doing it
unless really needed.

Co-developed-by: Heikki Krogerus <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
Co-developed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Co-developed-by: Raymond Tan <[email protected]>
Signed-off-by: Raymond Tan <[email protected]>
Co-developed-by: Lakshmi Sowjanya <[email protected]>
Signed-off-by: Lakshmi Sowjanya <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dwlib.c | 102 ++++++++++++++++++++++++++-
1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index d1ff3daeb0ba..da330ef46446 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -3,8 +3,10 @@

#include <linux/bitops.h>
#include <linux/bitfield.h>
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/kernel.h>
+#include <linux/math.h>
#include <linux/property.h>
#include <linux/serial_8250.h>
#include <linux/serial_core.h>
@@ -16,9 +18,18 @@
#define DW_UART_DE_EN 0xb0 /* Driver Output Enable Register */
#define DW_UART_RE_EN 0xb4 /* Receiver Output Enable Register */
#define DW_UART_DLF 0xc0 /* Divisor Latch Fraction Register */
+#define DW_UART_RAR 0xc4 /* Receive Address Register */
+#define DW_UART_TAR 0xc8 /* Transmit Address Register */
+#define DW_UART_LCR_EXT 0xcc /* Line Extended Control Register */
#define DW_UART_CPR 0xf4 /* Component Parameter Register */
#define DW_UART_UCV 0xf8 /* UART Component Version */

+/* Receive / Transmit Address Register bits */
+#define DW_UART_ADDR_MASK GENMASK(7, 0)
+
+/* Line Status Register bits */
+#define DW_UART_LSR_ADDR_RCVD BIT(8)
+
/* Transceiver Control Register bits */
#define DW_UART_TCR_RS485_EN BIT(0)
#define DW_UART_TCR_RE_POL BIT(1)
@@ -28,6 +39,12 @@
#define DW_UART_TCR_XFER_MODE_SW_DE_OR_RE FIELD_PREP(DW_UART_TCR_XFER_MODE, 1)
#define DW_UART_TCR_XFER_MODE_DE_OR_RE FIELD_PREP(DW_UART_TCR_XFER_MODE, 2)

+/* Line Extended Control Register bits */
+#define DW_UART_LCR_EXT_DLS_E BIT(0)
+#define DW_UART_LCR_EXT_ADDR_MATCH BIT(1)
+#define DW_UART_LCR_EXT_SEND_ADDR BIT(2)
+#define DW_UART_LCR_EXT_TRANSMIT_MODE BIT(3)
+
/* Component Parameter Register bits */
#define DW_UART_CPR_ABP_DATA_WIDTH (3 << 0)
#define DW_UART_CPR_AFCE_MODE (1 << 4)
@@ -82,9 +99,83 @@ void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct
p->status |= UPSTAT_AUTOCTS;

serial8250_do_set_termios(p, termios, old);
+
+ /* Filter addresses which have 9th bit set */
+ p->ignore_status_mask |= DW_UART_LSR_ADDR_RCVD;
+ p->read_status_mask |= DW_UART_LSR_ADDR_RCVD;
}
EXPORT_SYMBOL_GPL(dw8250_do_set_termios);

+/*
+ * Wait until re is de-asserted for sure. An ongoing receive will keep
+ * re asserted until end of frame. Without BUSY indication available,
+ * only available course of action is to wait for the time it takes to
+ * receive one frame (there might nothing to receive but w/o BUSY the
+ * driver cannot know).
+ */
+static void dw8250_wait_re_deassert(struct uart_port *p)
+{
+ ndelay(p->frame_time);
+}
+
+static void dw8250_update_rar(struct uart_port *p, u32 addr)
+{
+ u32 re_en = dw8250_readl_ext(p, DW_UART_RE_EN);
+
+ /*
+ * RAR shouldn't be changed while receiving. Thus, de-assert RE_EN
+ * if asserted and wait.
+ */
+ if (re_en)
+ dw8250_writel_ext(p, DW_UART_RE_EN, 0);
+ dw8250_wait_re_deassert(p);
+ dw8250_writel_ext(p, DW_UART_RAR, addr);
+ if (re_en)
+ dw8250_writel_ext(p, DW_UART_RE_EN, re_en);
+}
+
+static void dw8250_rs485_set_addr(struct uart_port *p, struct serial_rs485 *rs485,
+ struct ktermios *termios)
+{
+ u32 lcr = dw8250_readl_ext(p, DW_UART_LCR_EXT);
+
+ if (rs485->flags & SER_RS485_ADDRB) {
+ lcr |= DW_UART_LCR_EXT_DLS_E;
+ if (termios)
+ termios->c_cflag |= ADDRB;
+
+ if (rs485->flags & SER_RS485_ADDR_RECV) {
+ u32 delta = p->rs485.flags ^ rs485->flags;
+
+ /*
+ * rs485 (param) is equal to uart_port's rs485 only during init
+ * (during init, delta is not yet applicable).
+ */
+ if (unlikely(&p->rs485 == rs485))
+ delta = rs485->flags;
+
+ if ((delta & SER_RS485_ADDR_RECV) ||
+ (p->rs485.addr_recv != rs485->addr_recv))
+ dw8250_update_rar(p, rs485->addr_recv);
+ lcr |= DW_UART_LCR_EXT_ADDR_MATCH;
+ } else {
+ lcr &= ~DW_UART_LCR_EXT_ADDR_MATCH;
+ }
+ if (rs485->flags & SER_RS485_ADDR_DEST) {
+ /*
+ * Don't skip writes here as another endpoint could
+ * have changed communication line's destination
+ * address in between.
+ */
+ dw8250_writel_ext(p, DW_UART_TAR, rs485->addr_dest);
+ lcr |= DW_UART_LCR_EXT_SEND_ADDR;
+ }
+ } else {
+ lcr = 0;
+ }
+ dw8250_writel_ext(p, DW_UART_LCR_EXT, lcr);
+}
+
static int dw8250_rs485_config(struct uart_port *p, struct ktermios *termios,
struct serial_rs485 *rs485)
{
@@ -109,6 +200,9 @@ static int dw8250_rs485_config(struct uart_port *p, struct ktermios *termios,
dw8250_writel_ext(p, DW_UART_DE_EN, 1);
dw8250_writel_ext(p, DW_UART_RE_EN, 1);
} else {
+ if (termios)
+ termios->c_cflag &= ~ADDRB;
+
tcr &= ~DW_UART_TCR_RS485_EN;
}

@@ -123,6 +217,10 @@ static int dw8250_rs485_config(struct uart_port *p, struct ktermios *termios,

dw8250_writel_ext(p, DW_UART_TCR, tcr);

+ /* Addressing mode can only be set up after TCR */
+ if (rs485->flags & SER_RS485_ENABLED)
+ dw8250_rs485_set_addr(p, rs485, termios);
+
return 0;
}

@@ -142,7 +240,8 @@ static bool dw8250_detect_rs485_hw(struct uart_port *p)

static const struct serial_rs485 dw8250_rs485_supported = {
.flags = SER_RS485_ENABLED | SER_RS485_RX_DURING_TX | SER_RS485_RTS_ON_SEND |
- SER_RS485_RTS_AFTER_SEND,
+ SER_RS485_RTS_AFTER_SEND | SER_RS485_ADDRB | SER_RS485_ADDR_RECV |
+ SER_RS485_ADDR_DEST,
};

void dw8250_setup_port(struct uart_port *p)
@@ -155,6 +254,7 @@ void dw8250_setup_port(struct uart_port *p)
pd->hw_rs485_support = dw8250_detect_rs485_hw(p);
if (pd->hw_rs485_support) {
p->rs485_config = dw8250_rs485_config;
+ up->lsr_save_mask = LSR_SAVE_FLAGS | DW_UART_LSR_ADDR_RCVD;
p->rs485_supported = &dw8250_rs485_supported;
} else {
p->rs485_config = serial8250_em485_config;
--
2.30.2

2022-06-24 21:08:43

by Ilpo Järvinen

[permalink] [raw]
Subject: [PATCH v9 1/6] serial: 8250: make saved LSR larger

DW flags address received as BIT(8) in LSR. In order to not lose that
on read, enlarge lsr_saved_flags to u16.

Adjust lsr/status variables and related call chains to use u16.
Technically, some of these type conversion would not be needed but it
doesn't hurt to be consistent.

Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250.h | 4 ++--
drivers/tty/serial/8250/8250_exar.c | 2 +-
drivers/tty/serial/8250/8250_fsl.c | 2 +-
drivers/tty/serial/8250/8250_ingenic.c | 2 +-
drivers/tty/serial/8250/8250_omap.c | 7 +++----
drivers/tty/serial/8250/8250_port.c | 17 +++++++++--------
include/linux/serial_8250.h | 6 +++---
7 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b120da57c61f..0ff5688ba90c 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -133,9 +133,9 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
*
* Returns LSR value or'ed with the preserved flags (if any).
*/
-static inline unsigned int serial_lsr_in(struct uart_8250_port *up)
+static inline u16 serial_lsr_in(struct uart_8250_port *up)
{
- unsigned int lsr = up->lsr_saved_flags;
+ u16 lsr = up->lsr_saved_flags;

lsr |= serial_in(up, UART_LSR);
up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 528779b40049..3d999eec4087 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -195,11 +195,11 @@ static int xr17v35x_startup(struct uart_port *port)

static void exar_shutdown(struct uart_port *port)
{
- unsigned char lsr;
bool tx_complete = false;
struct uart_8250_port *up = up_to_u8250p(port);
struct circ_buf *xmit = &port->state->xmit;
int i = 0;
+ u16 lsr;

do {
lsr = serial_in(up, UART_LSR);
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 9c01c531349d..fd4005fcd0d6 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -25,8 +25,8 @@

int fsl8250_handle_irq(struct uart_port *port)
{
- unsigned char lsr, orig_lsr;
unsigned long flags;
+ u16 lsr, orig_lsr;
unsigned int iir;
struct uart_8250_port *up = up_to_u8250p(port);

diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
index cff91aa03f29..2b2f5d8d24b9 100644
--- a/drivers/tty/serial/8250/8250_ingenic.c
+++ b/drivers/tty/serial/8250/8250_ingenic.c
@@ -54,7 +54,7 @@ static void early_out(struct uart_port *port, int offset, uint8_t value)

static void ingenic_early_console_putc(struct uart_port *port, unsigned char c)
{
- uint8_t lsr;
+ u16 lsr;

do {
lsr = early_in(port, UART_LSR);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index ac8bfa042391..0dcecbbc3967 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1115,8 +1115,7 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
return omap_8250_rx_dma(up);
}

-static unsigned char omap_8250_handle_rx_dma(struct uart_8250_port *up,
- u8 iir, unsigned char status)
+static u16 omap_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir, u16 status)
{
if ((status & (UART_LSR_DR | UART_LSR_BI)) &&
(iir & UART_IIR_RDI)) {
@@ -1130,7 +1129,7 @@ static unsigned char omap_8250_handle_rx_dma(struct uart_8250_port *up,
}

static void am654_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir,
- unsigned char status)
+ u16 status)
{
/*
* Queue a new transfer if FIFO has data.
@@ -1164,7 +1163,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
struct omap8250_priv *priv = up->port.private_data;
- unsigned char status;
+ u16 status;
u8 iir;

serial8250_rpm_get(up);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index c860f5964138..19c612d732cf 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1508,7 +1508,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
struct uart_8250_em485 *em485 = p->em485;

if (em485) {
- unsigned char lsr = serial_lsr_in(p);
+ u16 lsr = serial_lsr_in(p);
u64 stop_delay = 0;

if (!(lsr & UART_LSR_THRE))
@@ -1565,7 +1565,7 @@ static inline void __start_tx(struct uart_port *port)

if (serial8250_set_THRI(up)) {
if (up->bugs & UART_BUG_TXEN) {
- unsigned char lsr = serial_lsr_in(up);
+ u16 lsr = serial_lsr_in(up);

if (lsr & UART_LSR_THRE)
serial8250_tx_chars(up);
@@ -1719,7 +1719,7 @@ static void serial8250_enable_ms(struct uart_port *port)
serial8250_rpm_put(up);
}

-void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
+void serial8250_read_char(struct uart_8250_port *up, u16 lsr)
{
struct uart_port *port = &up->port;
unsigned char ch;
@@ -1788,7 +1788,7 @@ EXPORT_SYMBOL_GPL(serial8250_read_char);
* (such as THRE) because the LSR value might come from an already consumed
* character.
*/
-unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
+u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr)
{
struct uart_port *port = &up->port;
int max_count = 256;
@@ -1908,10 +1908,10 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
*/
int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
{
- unsigned char status;
struct uart_8250_port *up = up_to_u8250p(port);
bool skip_rx = false;
unsigned long flags;
+ u16 status;

if (iir & UART_IIR_NO_INT)
return 0;
@@ -1994,7 +1994,7 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
unsigned long flags;
- unsigned int lsr;
+ u16 lsr;

serial8250_rpm_get(up);

@@ -2117,8 +2117,8 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
static int serial8250_get_poll_char(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
- unsigned char lsr;
int status;
+ u16 lsr;

serial8250_rpm_get(up);

@@ -2173,8 +2173,9 @@ int serial8250_do_startup(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
unsigned long flags;
- unsigned char lsr, iir;
+ unsigned char iir;
int retval;
+ u16 lsr;

if (!port->fifosize)
port->fifosize = uart_config[port->type].fifo_size;
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index ff84a3ed10ea..4565f25ba9a2 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -119,7 +119,7 @@ struct uart_8250_port {
* be immediately processed.
*/
#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
- unsigned char lsr_saved_flags;
+ u16 lsr_saved_flags;
#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
unsigned char msr_saved_flags;

@@ -170,8 +170,8 @@ extern void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
unsigned int quot_frac);
extern int fsl8250_handle_irq(struct uart_port *port);
int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
-unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
-void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
+u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr);
+void serial8250_read_char(struct uart_8250_port *up, u16 lsr);
void serial8250_tx_chars(struct uart_8250_port *up);
unsigned int serial8250_modem_status(struct uart_8250_port *up);
void serial8250_init_port(struct uart_8250_port *up);
--
2.30.2

2022-06-28 11:19:36

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v9 1/6] serial: 8250: make saved LSR larger

On Fri, Jun 24, 2022 at 11:42:05PM +0300, Ilpo J?rvinen wrote:
> DW flags address received as BIT(8) in LSR. In order to not lose that
> on read, enlarge lsr_saved_flags to u16.
>
> Adjust lsr/status variables and related call chains to use u16.
> Technically, some of these type conversion would not be needed but it
> doesn't hurt to be consistent.

Reviewed-by: Andy Shevchenko <[email protected]>

> Signed-off-by: Ilpo J?rvinen <[email protected]>
> ---
> drivers/tty/serial/8250/8250.h | 4 ++--
> drivers/tty/serial/8250/8250_exar.c | 2 +-
> drivers/tty/serial/8250/8250_fsl.c | 2 +-
> drivers/tty/serial/8250/8250_ingenic.c | 2 +-
> drivers/tty/serial/8250/8250_omap.c | 7 +++----
> drivers/tty/serial/8250/8250_port.c | 17 +++++++++--------
> include/linux/serial_8250.h | 6 +++---
> 7 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index b120da57c61f..0ff5688ba90c 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -133,9 +133,9 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
> *
> * Returns LSR value or'ed with the preserved flags (if any).
> */
> -static inline unsigned int serial_lsr_in(struct uart_8250_port *up)
> +static inline u16 serial_lsr_in(struct uart_8250_port *up)
> {
> - unsigned int lsr = up->lsr_saved_flags;
> + u16 lsr = up->lsr_saved_flags;
>
> lsr |= serial_in(up, UART_LSR);
> up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 528779b40049..3d999eec4087 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -195,11 +195,11 @@ static int xr17v35x_startup(struct uart_port *port)
>
> static void exar_shutdown(struct uart_port *port)
> {
> - unsigned char lsr;
> bool tx_complete = false;
> struct uart_8250_port *up = up_to_u8250p(port);
> struct circ_buf *xmit = &port->state->xmit;
> int i = 0;
> + u16 lsr;
>
> do {
> lsr = serial_in(up, UART_LSR);
> diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
> index 9c01c531349d..fd4005fcd0d6 100644
> --- a/drivers/tty/serial/8250/8250_fsl.c
> +++ b/drivers/tty/serial/8250/8250_fsl.c
> @@ -25,8 +25,8 @@
>
> int fsl8250_handle_irq(struct uart_port *port)
> {
> - unsigned char lsr, orig_lsr;
> unsigned long flags;
> + u16 lsr, orig_lsr;
> unsigned int iir;
> struct uart_8250_port *up = up_to_u8250p(port);
>
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index cff91aa03f29..2b2f5d8d24b9 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -54,7 +54,7 @@ static void early_out(struct uart_port *port, int offset, uint8_t value)
>
> static void ingenic_early_console_putc(struct uart_port *port, unsigned char c)
> {
> - uint8_t lsr;
> + u16 lsr;
>
> do {
> lsr = early_in(port, UART_LSR);
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index ac8bfa042391..0dcecbbc3967 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1115,8 +1115,7 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
> return omap_8250_rx_dma(up);
> }
>
> -static unsigned char omap_8250_handle_rx_dma(struct uart_8250_port *up,
> - u8 iir, unsigned char status)
> +static u16 omap_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir, u16 status)
> {
> if ((status & (UART_LSR_DR | UART_LSR_BI)) &&
> (iir & UART_IIR_RDI)) {
> @@ -1130,7 +1129,7 @@ static unsigned char omap_8250_handle_rx_dma(struct uart_8250_port *up,
> }
>
> static void am654_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir,
> - unsigned char status)
> + u16 status)
> {
> /*
> * Queue a new transfer if FIFO has data.
> @@ -1164,7 +1163,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
> struct omap8250_priv *priv = up->port.private_data;
> - unsigned char status;
> + u16 status;
> u8 iir;
>
> serial8250_rpm_get(up);
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index c860f5964138..19c612d732cf 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1508,7 +1508,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
> struct uart_8250_em485 *em485 = p->em485;
>
> if (em485) {
> - unsigned char lsr = serial_lsr_in(p);
> + u16 lsr = serial_lsr_in(p);
> u64 stop_delay = 0;
>
> if (!(lsr & UART_LSR_THRE))
> @@ -1565,7 +1565,7 @@ static inline void __start_tx(struct uart_port *port)
>
> if (serial8250_set_THRI(up)) {
> if (up->bugs & UART_BUG_TXEN) {
> - unsigned char lsr = serial_lsr_in(up);
> + u16 lsr = serial_lsr_in(up);
>
> if (lsr & UART_LSR_THRE)
> serial8250_tx_chars(up);
> @@ -1719,7 +1719,7 @@ static void serial8250_enable_ms(struct uart_port *port)
> serial8250_rpm_put(up);
> }
>
> -void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
> +void serial8250_read_char(struct uart_8250_port *up, u16 lsr)
> {
> struct uart_port *port = &up->port;
> unsigned char ch;
> @@ -1788,7 +1788,7 @@ EXPORT_SYMBOL_GPL(serial8250_read_char);
> * (such as THRE) because the LSR value might come from an already consumed
> * character.
> */
> -unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
> +u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr)
> {
> struct uart_port *port = &up->port;
> int max_count = 256;
> @@ -1908,10 +1908,10 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
> */
> int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
> {
> - unsigned char status;
> struct uart_8250_port *up = up_to_u8250p(port);
> bool skip_rx = false;
> unsigned long flags;
> + u16 status;
>
> if (iir & UART_IIR_NO_INT)
> return 0;
> @@ -1994,7 +1994,7 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
> unsigned long flags;
> - unsigned int lsr;
> + u16 lsr;
>
> serial8250_rpm_get(up);
>
> @@ -2117,8 +2117,8 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> static int serial8250_get_poll_char(struct uart_port *port)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
> - unsigned char lsr;
> int status;
> + u16 lsr;
>
> serial8250_rpm_get(up);
>
> @@ -2173,8 +2173,9 @@ int serial8250_do_startup(struct uart_port *port)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
> unsigned long flags;
> - unsigned char lsr, iir;
> + unsigned char iir;
> int retval;
> + u16 lsr;
>
> if (!port->fifosize)
> port->fifosize = uart_config[port->type].fifo_size;
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index ff84a3ed10ea..4565f25ba9a2 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -119,7 +119,7 @@ struct uart_8250_port {
> * be immediately processed.
> */
> #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
> - unsigned char lsr_saved_flags;
> + u16 lsr_saved_flags;
> #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
> unsigned char msr_saved_flags;
>
> @@ -170,8 +170,8 @@ extern void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
> unsigned int quot_frac);
> extern int fsl8250_handle_irq(struct uart_port *port);
> int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
> -unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
> -void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
> +u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr);
> +void serial8250_read_char(struct uart_8250_port *up, u16 lsr);
> void serial8250_tx_chars(struct uart_8250_port *up);
> unsigned int serial8250_modem_status(struct uart_8250_port *up);
> void serial8250_init_port(struct uart_8250_port *up);
> --
> 2.30.2
>

--
With Best Regards,
Andy Shevchenko


2022-09-20 11:48:11

by Lukas Wunner

[permalink] [raw]
Subject: Re: [PATCH v9 5/6] serial: Support for RS-485 multipoint addresses

On Fri, Jun 24, 2022 at 11:42:09PM +0300, Ilpo J?rvinen wrote:
> ADDRB in termios indicates 9th bit addressing mode is enabled. In this
> mode, 9th bit is used to indicate an address (byte) within the
> communication line. ADDRB can only be enabled/disabled through
> ->rs485_config() that is also responsible for setting the destination and
> receiver (filter) addresses.
[...]
> --- a/include/uapi/asm-generic/termbits-common.h
> +++ b/include/uapi/asm-generic/termbits-common.h
> @@ -46,6 +46,7 @@ typedef unsigned int speed_t;
> #define EXTA B19200
> #define EXTB B38400
>
> +#define ADDRB 0x20000000 /* address bit */
> #define CMSPAR 0x40000000 /* mark or space (stick) parity */
> #define CRTSCTS 0x80000000 /* flow control */
>

You may want to consider submitting a patch to the Linux man-pages
project to document the newly introduced bit:

https://www.kernel.org/doc/man-pages/patches.html
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man3/termios.3

Thanks,

Lukas