[PATCH v7 1/6] serial: 8250: make saved LSR larger

[PATCH v7 1/6] serial: 8250: make saved LSR larger

[Ilpo Järvinen]

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 <ilpo.jarvinen@linux.intel.com>
---
 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..8be3c9ecd623 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -195,7 +195,7 @@ static int xr17v35x_startup(struct uart_port *port)
 
 static void exar_shutdown(struct uart_port *port)
 {
-	unsigned char lsr;
+	u16 lsr;
 	bool tx_complete = false;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct circ_buf *xmit = &port->state->xmit;
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 9c01c531349d..e76b8bd3d4a1 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -25,7 +25,7 @@
 
 int fsl8250_handle_irq(struct uart_port *port)
 {
-	unsigned char lsr, orig_lsr;
+	u16 lsr, orig_lsr;
 	unsigned long flags;
 	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..f417c29507e1 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,7 +1908,7 @@ 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;
+	u16 status;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	bool skip_rx = false;
 	unsigned long flags;
@@ -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,7 +2117,7 @@ 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;
+	u16 lsr;
 	int status;
 
 	serial8250_rpm_get(up);
@@ -2173,7 +2173,8 @@ 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;
+	u16 lsr;
 	int retval;
 
 	if (!port->fifosize)
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

Re: [PATCH v7 1/6] serial: 8250: make saved LSR larger

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 03:48:24PM +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.

...

>  static void exar_shutdown(struct uart_port *port)
>  {
> -	unsigned char lsr;
> +	u16 lsr;

I would take a chance and move it under the longer line(s), like after xmit
(taking into account given context).

>  	bool tx_complete = false;
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	struct circ_buf *xmit = &port->state->xmit;

...

>  int fsl8250_handle_irq(struct uart_port *port)
>  {
> -	unsigned char lsr, orig_lsr;
> +	u16 lsr, orig_lsr;

Ditto. And so on.

>  	unsigned long flags;
>  	unsigned int iir;
>  	struct uart_8250_port *up = up_to_u8250p(port);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v7 2/6] serial: 8250: create lsr_save_mask

[Ilpo Järvinen]

Allow drivers to alter LSR save mask.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250.h      | 2 +-
 drivers/tty/serial/8250/8250_core.c | 4 ++++
 drivers/tty/serial/8250/8250_dw.c   | 2 +-
 include/linux/serial_8250.h         | 1 +
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 0ff5688ba90c..5cc967fe3b59 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -138,7 +138,7 @@ static inline u16 serial_lsr_in(struct uart_8250_port *up)
 	u16 lsr = up->lsr_saved_flags;
 
 	lsr |= serial_in(up, UART_LSR);
-	up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
+	up->lsr_saved_flags = lsr & up->lsr_save_mask;
 
 	return lsr;
 }
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 90ddc8924811..57e86133af4f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1007,6 +1007,7 @@ 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->lsr_save_mask	= up->lsr_save_mask;
 		uart->dma		= up->dma;
 
 		/* Take tx_loadsz from fifosize if it wasn't set separately */
@@ -1094,6 +1095,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 			ret = 0;
 		}
 
+		if (!uart->lsr_save_mask)
+			uart->lsr_save_mask = LSR_SAVE_FLAGS;	/* Use default LSR mask */
+
 		/* Initialise interrupt backoff work if required */
 		if (up->overrun_backoff_time_ms > 0) {
 			uart->overrun_backoff_time_ms =
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 4cc69bb612ab..167a691c7b19 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -129,7 +129,7 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 
 	while (tries--) {
 		lsr = readb (p->membase + (UART_LSR << p->regshift));
-		up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+		up->lsr_saved_flags |= lsr & up->lsr_save_mask;
 
 		if (lsr & UART_LSR_TEMT)
 			break;
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 4565f25ba9a2..8c7b793aa4d7 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -120,6 +120,7 @@ struct uart_8250_port {
 	 */
 #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
 	u16			lsr_saved_flags;
+	u16			lsr_save_mask;
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
 
-- 
2.30.2

Re: [PATCH v7 2/6] serial: 8250: create lsr_save_mask

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 03:48:25PM +0300, Ilpo Järvinen wrote:
> Allow drivers to alter LSR save mask.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250.h      | 2 +-
>  drivers/tty/serial/8250/8250_core.c | 4 ++++
>  drivers/tty/serial/8250/8250_dw.c   | 2 +-
>  include/linux/serial_8250.h         | 1 +
>  4 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 0ff5688ba90c..5cc967fe3b59 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -138,7 +138,7 @@ static inline u16 serial_lsr_in(struct uart_8250_port *up)
>  	u16 lsr = up->lsr_saved_flags;
>  
>  	lsr |= serial_in(up, UART_LSR);
> -	up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
> +	up->lsr_saved_flags = lsr & up->lsr_save_mask;
>  
>  	return lsr;
>  }
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 90ddc8924811..57e86133af4f 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1007,6 +1007,7 @@ 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->lsr_save_mask	= up->lsr_save_mask;
>  		uart->dma		= up->dma;
>  
>  		/* Take tx_loadsz from fifosize if it wasn't set separately */
> @@ -1094,6 +1095,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  			ret = 0;
>  		}
>  
> +		if (!uart->lsr_save_mask)
> +			uart->lsr_save_mask = LSR_SAVE_FLAGS;	/* Use default LSR mask */
> +
>  		/* Initialise interrupt backoff work if required */
>  		if (up->overrun_backoff_time_ms > 0) {
>  			uart->overrun_backoff_time_ms =
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 4cc69bb612ab..167a691c7b19 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -129,7 +129,7 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
>  
>  	while (tries--) {
>  		lsr = readb (p->membase + (UART_LSR << p->regshift));
> -		up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> +		up->lsr_saved_flags |= lsr & up->lsr_save_mask;
>  
>  		if (lsr & UART_LSR_TEMT)
>  			break;
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 4565f25ba9a2..8c7b793aa4d7 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -120,6 +120,7 @@ struct uart_8250_port {
>  	 */
>  #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
>  	u16			lsr_saved_flags;
> +	u16			lsr_save_mask;
>  #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>  	unsigned char		msr_saved_flags;
>  
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v7 3/6] serial: 8250_lpss: Use 32-bit reads

[Ilpo Järvinen]

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 <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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

[PATCH v7 4/6] serial: take termios_rwsem for .rs485_config() & pass termios as param

[Ilpo Järvinen]

To be able to alter ADDRB within .rs485_config(), take termios_rwsem
before calling .rs485_config() and pass termios.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250.h         |  3 ++-
 drivers/tty/serial/8250/8250_dwlib.c   |  3 ++-
 drivers/tty/serial/8250/8250_exar.c    |  9 +++++----
 drivers/tty/serial/8250/8250_fintek.c  |  2 +-
 drivers/tty/serial/8250/8250_lpc18xx.c |  2 +-
 drivers/tty/serial/8250/8250_pci.c     |  2 +-
 drivers/tty/serial/8250/8250_port.c    |  3 ++-
 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       | 15 +++++++++++----
 drivers/tty/serial/stm32-usart.c       |  2 +-
 include/linux/serial_core.h            |  3 ++-
 19 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 5cc967fe3b59..e7ba6500aedf 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -203,7 +203,8 @@ void serial8250_rpm_put(struct uart_8250_port *p);
 void serial8250_rpm_get_tx(struct uart_8250_port *p);
 void serial8250_rpm_put_tx(struct uart_8250_port *p);
 
-int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485);
+int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485,
+			    struct ktermios *termios);
 void serial8250_em485_start_tx(struct uart_8250_port *p);
 void serial8250_em485_stop_tx(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index c83e7eaf3877..c9d9bd7f7bd9 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -85,7 +85,8 @@ void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, struct
 }
 EXPORT_SYMBOL_GPL(dw8250_do_set_termios);
 
-static int dw8250_rs485_config(struct uart_port *p, struct serial_rs485 *rs485)
+static int dw8250_rs485_config(struct uart_port *p, struct serial_rs485 *rs485,
+			       struct ktermios *termios)
 {
 	u32 tcr;
 
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 8be3c9ecd623..ff67d154a7b5 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -112,7 +112,8 @@
 struct exar8250;
 
 struct exar8250_platform {
-	int (*rs485_config)(struct uart_port *, struct serial_rs485 *);
+	int (*rs485_config)(struct uart_port *port, struct serial_rs485 *rs485,
+			    struct ktermios *termios);
 	const struct serial_rs485 *rs485_supported;
 	int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
 	void (*unregister_gpio)(struct uart_8250_port *);
@@ -410,7 +411,7 @@ static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
 }
 
 static int generic_rs485_config(struct uart_port *port,
-				struct serial_rs485 *rs485)
+				struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
 	u8 __iomem *p = port->membase;
@@ -442,7 +443,7 @@ static const struct exar8250_platform exar8250_default_platform = {
 };
 
 static int iot2040_rs485_config(struct uart_port *port,
-				struct serial_rs485 *rs485)
+				struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
 	u8 __iomem *p = port->membase;
@@ -471,7 +472,7 @@ static int iot2040_rs485_config(struct uart_port *port,
 	value |= mode;
 	writeb(value, p + UART_EXAR_MPIOLVL_7_0);
 
-	return generic_rs485_config(port, rs485);
+	return generic_rs485_config(port, rs485, termios);
 }
 
 static const struct serial_rs485 iot2040_rs485_supported = {
diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index 1fb86c73786c..e48d52534bef 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -192,7 +192,7 @@ static int fintek_8250_get_ldn_range(struct fintek_8250 *pdata, int *min,
 }
 
 static int fintek_8250_rs485_config(struct uart_port *port,
-			      struct serial_rs485 *rs485)
+			      struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	uint8_t config = 0;
 	struct fintek_8250 *pdata = port->private_data;
diff --git a/drivers/tty/serial/8250/8250_lpc18xx.c b/drivers/tty/serial/8250/8250_lpc18xx.c
index 3a1cb51cbc91..1865f7bc5020 100644
--- a/drivers/tty/serial/8250/8250_lpc18xx.c
+++ b/drivers/tty/serial/8250/8250_lpc18xx.c
@@ -33,7 +33,7 @@ struct lpc18xx_uart_data {
 };
 
 static int lpc18xx_rs485_config(struct uart_port *port,
-				struct serial_rs485 *rs485)
+				struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	u32 rs485_ctrl_reg = 0;
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index b6d71268aa7d..35c799641e32 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1554,7 +1554,7 @@ pci_brcm_trumanage_setup(struct serial_private *priv,
 
 /* We should do proper H/W transceiver setting before change to RS485 mode */
 static int pci_fintek_rs485_config(struct uart_port *port,
-			       struct serial_rs485 *rs485)
+				   struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct pci_dev *pci_dev = to_pci_dev(port->dev);
 	u8 setting;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f417c29507e1..0ec06995df89 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -664,7 +664,8 @@ EXPORT_SYMBOL_GPL(serial8250_em485_supported);
  * if the uart is incapable of driving RTS as a Transmit Enable signal in
  * hardware, relying on software emulation instead.
  */
-int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485)
+int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485,
+			    struct ktermios *termios)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index eccd66625d25..aefb0600a82c 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2198,7 +2198,7 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
 }
 
 static int pl011_rs485_config(struct uart_port *port,
-			      struct serial_rs485 *rs485)
+			      struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct uart_amba_port *uap =
 		container_of(port, struct uart_amba_port, port);
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index ab2c5b2a1ce8..930d9aed90f5 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -581,7 +581,7 @@ static const struct uart_ops ar933x_uart_ops = {
 };
 
 static int ar933x_config_rs485(struct uart_port *port,
-				struct serial_rs485 *rs485conf)
+			       struct serial_rs485 *rs485conf, struct ktermios *termios)
 {
 	struct ar933x_uart_port *up =
 		container_of(port, struct ar933x_uart_port, port);
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 74dd1d3ac46f..b032e7c30ce0 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -286,7 +286,7 @@ static void atmel_tasklet_schedule(struct atmel_uart_port *atmel_port,
 
 /* Enable or disable the rs485 support */
 static int atmel_config_rs485(struct uart_port *port,
-			      struct serial_rs485 *rs485conf)
+			      struct serial_rs485 *rs485conf, struct ktermios *termios)
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	unsigned int mode;
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index d35414cb3e4e..3e8da24a3f76 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1356,7 +1356,7 @@ static void lpuart_dma_rx_free(struct uart_port *port)
 }
 
 static int lpuart_config_rs485(struct uart_port *port,
-			struct serial_rs485 *rs485)
+			       struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct lpuart_port *sport = container_of(port,
 			struct lpuart_port, port);
@@ -1386,7 +1386,7 @@ static int lpuart_config_rs485(struct uart_port *port,
 }
 
 static int lpuart32_config_rs485(struct uart_port *port,
-			struct serial_rs485 *rs485)
+				 struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct lpuart_port *sport = container_of(port,
 			struct lpuart_port, port);
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index f4edde54175f..8a63ab6e0d8e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1908,7 +1908,7 @@ static void imx_uart_poll_put_char(struct uart_port *port, unsigned char c)
 
 /* called with port.lock taken and irqs off or from .probe without locking */
 static int imx_uart_rs485_config(struct uart_port *port,
-				 struct serial_rs485 *rs485conf)
+				 struct serial_rs485 *rs485conf, struct ktermios *termios)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	u32 ucr2;
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index d7d1791fcb57..0e1dfad03825 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1027,7 +1027,7 @@ static void max310x_rs_proc(struct work_struct *ws)
 }
 
 static int max310x_rs485_config(struct uart_port *port,
-				struct serial_rs485 *rs485)
+				struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 036f178e3d66..fbdf8c53ddd0 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -431,7 +431,8 @@ static int mcf_verify_port(struct uart_port *port, struct serial_struct *ser)
 /****************************************************************************/
 
 /* Enable or disable the RS485 support */
-static int mcf_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+static int mcf_config_rs485(struct uart_port *port, struct serial_rs485 *rs485,
+			    struct ktermios *termios)
 {
 	unsigned char mr1, mr2;
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 98622c35d896..07446bcbfa8c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1325,7 +1325,8 @@ static inline void serial_omap_add_console_port(struct uart_omap_port *up)
 
 /* Enable or disable the rs485 support */
 static int
-serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485,
+			 struct ktermios *termios)
 {
 	struct uart_omap_port *up = to_uart_omap_port(port);
 	unsigned int mode;
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 2ceecaa4a478..f84c693ef5e7 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1128,7 +1128,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 }
 
 static int sc16is7xx_config_rs485(struct uart_port *port,
-				  struct serial_rs485 *rs485)
+				  struct serial_rs485 *rs485, struct ktermios *termios)
 {
 	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 621fc15e2e54..76bb1b77b06e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1359,7 +1359,8 @@ int uart_rs485_config(struct uart_port *port)
 
 	uart_sanitize_serial_rs485(port, rs485);
 
-	ret = port->rs485_config(port, rs485);
+	ret = port->rs485_config(port, rs485, NULL);
+
 	if (ret)
 		memset(rs485, 0, sizeof(*rs485));
 
@@ -1383,7 +1384,7 @@ static int uart_get_rs485_config(struct uart_port *port,
 	return 0;
 }
 
-static int uart_set_rs485_config(struct uart_port *port,
+static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
 			 struct serial_rs485 __user *rs485_user)
 {
 	struct serial_rs485 rs485;
@@ -1402,7 +1403,7 @@ static int uart_set_rs485_config(struct uart_port *port,
 	uart_sanitize_serial_rs485(port, &rs485);
 
 	spin_lock_irqsave(&port->lock, flags);
-	ret = port->rs485_config(port, &rs485);
+	ret = port->rs485_config(port, &rs485, &tty->termios);
 	if (!ret)
 		port->rs485 = rs485;
 	spin_unlock_irqrestore(&port->lock, flags);
@@ -1511,6 +1512,10 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 	if (ret != -ENOIOCTLCMD)
 		goto out;
 
+	/* rs485_config requires more locking than others */
+	if (cmd == TIOCGRS485)
+		down_write(&tty->termios_rwsem);
+
 	mutex_lock(&port->mutex);
 	uport = uart_port_check(state);
 
@@ -1534,7 +1539,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 		break;
 
 	case TIOCSRS485:
-		ret = uart_set_rs485_config(uport, uarg);
+		ret = uart_set_rs485_config(tty, uport, uarg);
 		break;
 
 	case TIOCSISO7816:
@@ -1551,6 +1556,8 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 	}
 out_up:
 	mutex_unlock(&port->mutex);
+	if (cmd == TIOCGRS485)
+		up_write(&tty->termios_rwsem);
 out:
 	return ret;
 }
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index db3dd9731ee1..de8e3cbda4f3 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -98,7 +98,7 @@ static void stm32_usart_config_reg_rs485(u32 *cr1, u32 *cr3, u32 delay_ADE,
 }
 
 static int stm32_usart_config_rs485(struct uart_port *port,
-				    struct serial_rs485 *rs485conf)
+				    struct serial_rs485 *rs485conf, struct ktermios *termios)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
 	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5518b70177b3..9577cffb5593 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -132,7 +132,8 @@ struct uart_port {
 				      unsigned int old);
 	void			(*handle_break)(struct uart_port *);
 	int			(*rs485_config)(struct uart_port *,
-						struct serial_rs485 *rs485);
+						struct serial_rs485 *rs485,
+						struct ktermios *termios);
 	int			(*iso7816_config)(struct uart_port *,
 						  struct serial_iso7816 *iso7816);
 	unsigned int		irq;			/* irq number */
-- 
2.30.2

Re: [PATCH v7 4/6] serial: take termios_rwsem for .rs485_config() & pass termios as param

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 03:48:27PM +0300, Ilpo Järvinen wrote:
> To be able to alter ADDRB within .rs485_config(), take termios_rwsem
> before calling .rs485_config() and pass termios.

I would use ->rs485_config() as a reference to the callback.

...

> -	ret = port->rs485_config(port, rs485);
> +	ret = port->rs485_config(port, rs485, NULL);

> +

Stray change?

>  	if (ret)
>  		memset(rs485, 0, sizeof(*rs485));

...

>  	void			(*handle_break)(struct uart_port *);
>  	int			(*rs485_config)(struct uart_port *,
> -						struct serial_rs485 *rs485);
> +						struct serial_rs485 *rs485,
> +						struct ktermios *termios);

Dunno if termios has to be second parameter. The idea is to pass input data
followed by (auxiliary) output as usual pattern.

>  	int			(*iso7816_config)(struct uart_port *,
>  						  struct serial_iso7816 *iso7816);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/6] serial: take termios_rwsem for .rs485_config() & pass termios as param

[Ilpo Järvinen]

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

On Wed, 15 Jun 2022, Andy Shevchenko wrote:

> On Wed, Jun 15, 2022 at 03:48:27PM +0300, Ilpo Järvinen wrote:
> > To be able to alter ADDRB within .rs485_config(), take termios_rwsem
> > before calling .rs485_config() and pass termios.
> 
> I would use ->rs485_config() as a reference to the callback.
> 
> ...
> 
> > -	ret = port->rs485_config(port, rs485);
> > +	ret = port->rs485_config(port, rs485, NULL);
> 
> > +
> 
> Stray change?

Yes it was.

> >  	if (ret)
> >  		memset(rs485, 0, sizeof(*rs485));
> 
> ...
> 
> >  	void			(*handle_break)(struct uart_port *);
> >  	int			(*rs485_config)(struct uart_port *,
> > -						struct serial_rs485 *rs485);
> > +						struct serial_rs485 *rs485,
> > +						struct ktermios *termios);
> 
> Dunno if termios has to be second parameter. The idea is to pass input data
> followed by (auxiliary) output as usual pattern.

I guess I can make termios 2nd param.

-- 
 i.

[PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

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.

[*] 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: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-api@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 Documentation/driver-api/serial/driver.rst    |  2 ++
 .../driver-api/serial/serial-rs485.rst        | 26 ++++++++++++++++++-
 drivers/tty/serial/serial_core.c              | 11 ++++++++
 drivers/tty/tty_ioctl.c                       |  4 +++
 include/uapi/asm-generic/termbits-common.h    |  1 +
 include/uapi/linux/serial.h                   | 12 +++++++--
 6 files changed, 53 insertions(+), 3 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 76bb1b77b06e..b2bce1696540 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;
 }
 
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index adae687f654b..ed253f2337a7 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..9df86f0a3d46 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -126,10 +126,18 @@ 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 .. */
+	__u8	addr_recv;
+	__u8	addr_dest;
+	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
+							 * are a royal PITA .. */
 };
 
 /*
-- 
2.30.2

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> 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.
> 
> [*] 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: Arnd Bergmann <arnd@arndb.de>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-api@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arch@vger.kernel.org

Hmm... In order to reduce commit messages you can move these Cc:s after the
cutter line ('---').

...

> -	__u32	padding[5];		/* Memory is cheap, new structs
> -					   are a royal PITA .. */
> +	__u8	addr_recv;
> +	__u8	addr_dest;
> +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
> +							 * are a royal PITA .. */

I'm not sure it's an equivalent. I would leave u32 members  untouched, so
something like

	__u8	addr_recv;
	__u8	addr_dest;
	__u8	padding0[2];		/* Memory is cheap, new structs
	__u32	padding1[4];		 * are a royal PITA .. */

And repeating about `pahole` tool which may be useful here to check for ABI
potential changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

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

On Wed, 15 Jun 2022, Andy Shevchenko wrote:

> On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> > 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.
> > 
> > [*] 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: Arnd Bergmann <arnd@arndb.de>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: linux-api@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-arch@vger.kernel.org
> 
> Hmm... In order to reduce commit messages you can move these Cc:s after the
> cutter line ('---').

Ok, although the toolchain I use didn't support preserving --- content
so I had to create hack to preserve them, hopefully nothing backfires due 
to the hack. :-)

> > -	__u32	padding[5];		/* Memory is cheap, new structs
> > -					   are a royal PITA .. */
> > +	__u8	addr_recv;
> > +	__u8	addr_dest;
> > +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
> > +							 * are a royal PITA .. */
> 
> I'm not sure it's an equivalent. I would leave u32 members  untouched, so
> something like
> 
> 	__u8	addr_recv;
> 	__u8	addr_dest;
> 	__u8	padding0[2];		/* Memory is cheap, new structs
> 	__u32	padding1[4];		 * are a royal PITA .. */
> 
> And repeating about `pahole` tool which may be useful here to check for ABI
> potential changes.

I cannot take __u32 padding[] away like that, this is an uapi header. Or 
do you mean I should create anonymous union? ...I'm skeptical that can be 
pulled off w/o breaking user-space compile in some circumstances. Anon 
unions were only introduced by C11 but is it ok to rely on C11 in uapi/ 
headers?

Even making padding smaller has some unwanted consequences if somebody is 
clearing just .padding. In retrospect, having padding as a direct member 
doesn't seem a good idea. That padding[5] should have been within an union 
right from the start to make this easily extendable.

Maybe create a copy of that struct under another name which is just equal 
sized, that would give more freedom on member naming. But can I change 
ioctl's param type to another struct (in _IOR/_IOWR) w/o breaking 
something?

-- 
 i.

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Jiri Slaby]

On 16. 06. 22, 7:04, Ilpo Järvinen wrote:
> On Wed, 15 Jun 2022, Andy Shevchenko wrote:
> 
>> On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
>>> 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.
>>>
>>> [*] 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: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>> Cc: linux-api@vger.kernel.org
>>> Cc: linux-doc@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linux-arch@vger.kernel.org
>>
>> Hmm... In order to reduce commit messages you can move these Cc:s after the
>> cutter line ('---').
> 
> Ok, although the toolchain I use didn't support preserving --- content
> so I had to create hack to preserve them, hopefully nothing backfires due
> to the hack. :-)
> 
>>> -	__u32	padding[5];		/* Memory is cheap, new structs
>>> -					   are a royal PITA .. */
>>> +	__u8	addr_recv;
>>> +	__u8	addr_dest;
>>> +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
>>> +							 * are a royal PITA .. */
>>
>> I'm not sure it's an equivalent. I would leave u32 members  untouched, so
>> something like
>>
>> 	__u8	addr_recv;
>> 	__u8	addr_dest;
>> 	__u8	padding0[2];		/* Memory is cheap, new structs
>> 	__u32	padding1[4];		 * are a royal PITA .. */
>>
>> And repeating about `pahole` tool which may be useful here to check for ABI
>> potential changes.
> 
> I cannot take __u32 padding[] away like that, this is an uapi header.

Yeah, but it's padding after all. I would personally break it for 
example as Andy suggests (if pahole shows no differences in size on both 
32/64 bit) and wait if something breaks. To be honest, I'd not expect 
anyone to touch it. And if someone does, we would fix it somehow and 
they should too...

thanks,
-- 
js
suse labs

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

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

On Thu, 16 Jun 2022, Jiri Slaby wrote:

> On 16. 06. 22, 7:04, Ilpo Järvinen wrote:
> > On Wed, 15 Jun 2022, Andy Shevchenko wrote:
> > 
> > > On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> > > > 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.
> > > > 
> > > > [*] 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: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Jonathan Corbet <corbet@lwn.net>
> > > > Cc: linux-api@vger.kernel.org
> > > > Cc: linux-doc@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: linux-arch@vger.kernel.org
> > > 
> > > Hmm... In order to reduce commit messages you can move these Cc:s after
> > > the
> > > cutter line ('---').
> > 
> > Ok, although the toolchain I use didn't support preserving --- content
> > so I had to create hack to preserve them, hopefully nothing backfires due
> > to the hack. :-)
> > 
> > > > -	__u32	padding[5];		/* Memory is cheap, new structs
> > > > -					   are a royal PITA .. */
> > > > +	__u8	addr_recv;
> > > > +	__u8	addr_dest;
> > > > +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap,
> > > > new structs
> > > > +							 * are a royal PITA ..
> > > > */
> > > 
> > > I'm not sure it's an equivalent. I would leave u32 members  untouched, so
> > > something like
> > > 
> > > 	__u8	addr_recv;
> > > 	__u8	addr_dest;
> > > 	__u8	padding0[2];		/* Memory is cheap, new structs
> > > 	__u32	padding1[4];		 * are a royal PITA .. */
> > > 
> > > And repeating about `pahole` tool which may be useful here to check for
> > > ABI
> > > potential changes.
> > 
> > I cannot take __u32 padding[] away like that, this is an uapi header.
> 
> Yeah, but it's padding after all. I would personally break it for example as
> Andy suggests (if pahole shows no differences in size on both 32/64 bit) and
> wait if something breaks. To be honest, I'd not expect anyone to touch it. And
> if someone does, we would fix it somehow and they should too...

I realized there are plenty of anonymous unions already in include/uapi/ 
so I think I can keep padding[5] too:

        union {
                /* v1 */
                __u32   padding[5];             /* Memory is cheap, new structs are a pain */

                /* v2 (adds addressing mode fields) */
                struct {
                        __u8    addr_recv;
                        __u8    addr_dest;
                        __u8    padding0[2];
                        __u32   padding1[4];
                };
        };

I'll just skip manual pahole step and add a few BUILD_BUG_ON()s and use 
our build bot to do a quick check over all archs it builds for, that gives 
much better confidence on it being ok:

	BUILD_BUG_ON(((&rs485.delay_rts_after_send) + 1) != &rs485.padding[0]);
	BUILD_BUG_ON(&rs485.padding[1] != &rs485.padding1[0]);
	BUILD_BUG_ON(sizeof(rs485) != ((u8 *)(&rs485.padding[4]) - ((u8 *)&rs485.flags) + sizeof(__u32)));


-- 
 i.

[PATCH v7 6/6] serial: 8250_dwlib: Support for 9th bit multipoint addressing

[Ilpo Järvinen]

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 <heikki.krogerus@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Co-developed-by: Raymond Tan <raymond.tan@intel.com>
Signed-off-by: Raymond Tan <raymond.tan@intel.com>
Co-developed-by: Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>
Signed-off-by: Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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 c9d9bd7f7bd9..fe6feb59e68d 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 serial_rs485 *rs485,
 			       struct ktermios *termios)
 {
@@ -109,6 +200,9 @@ static int dw8250_rs485_config(struct uart_port *p, struct serial_rs485 *rs485,
 		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 serial_rs485 *rs485,
 
 	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