[PATCH v2 0/3] add support for EXAR XR20M1172 UART

[PATCH v2 0/3] add support for EXAR XR20M1172 UART

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

EXAR XR20M1172 UART is mostly SC16IS762-compatible, but
it has additional register which can change UART multiplier
to 4x and 8x, similar to UPF_MAGIC_MULTIPLIER does. So, I used this
flag to guard access to its specific DLD register. It seems than
other EXAR SPI UART modules also have this register, but I tested
only XR20M1172.

Changes in v2:
  use full name in git authorship

Konstantin Pugin (3):
  serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND
  serial: sc16is7xx: Add bindings documentation for EXAR XR20M1172 UART
  serial: sc16is7xx: add support for EXAR XR20M1172 UART

 .../bindings/serial/nxp,sc16is7xx.yaml        |  1 +
 drivers/tty/serial/sc16is7xx.c                | 56 +++++++++++++++++--
 2 files changed, 53 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

Hardware supports both modes, but after
commit 4afeced55baa ("serial: core: fix sanitizing check for RTS settings")
we always end up with SER_RS485_RTS_AFTER_SEND and always write in
register SC16IS7XX_EFCR_RTS_INVERT_BIT,
which breaks some hardware using these chips,

Fixes: 267913ecf737 ("serial: sc16is7xx: Fill in rs485_supported")
Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 929206a9a6e1..a300eebf1401 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1458,7 +1458,7 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
 }
 
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
 	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
 };
-- 
2.34.1

[PATCH v2 2/3] serial: sc16is7xx: Add bindings documentation for EXAR XR20M1172 UART

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

This patch adds the devicetree documentation for the XR20M1172 UART.

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
index 5dec15b7e7c3..68fe7b11961c 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
@@ -18,6 +18,7 @@ properties:
       - nxp,sc16is752
       - nxp,sc16is760
       - nxp,sc16is762
+      - exar,xr20m1172
 
   reg:
     maxItems: 1
-- 
2.34.1

[PATCH v2 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Konstantin Pugin]

From: Konstantin Pugin <ria.freelander@gmail.com>

Its register set is mostly compatible with SC16IS762, but
it has a support for additional division rates of UART
with special DLD register. So, add handling this register
via UPF_MAGIC_MULTIPLIER port flag.

Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 54 ++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a300eebf1401..7fc1c19b3891 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -65,6 +65,7 @@
 /* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
 #define SC16IS7XX_DLL_REG		(0x00) /* Divisor Latch Low */
 #define SC16IS7XX_DLH_REG		(0x01) /* Divisor Latch High */
+#define SC16IS7XX_DLD_REG		(0x02) /* Divisor Latch Mode (only on EXAR chips) */
 
 /* Enhanced Register set: Only if (LCR == 0xBF) */
 #define SC16IS7XX_EFR_REG		(0x02) /* Enhanced Features */
@@ -218,6 +219,20 @@
 #define SC16IS7XX_TCR_RX_HALT(words)	((((words) / 4) & 0x0f) << 0)
 #define SC16IS7XX_TCR_RX_RESUME(words)	((((words) / 4) & 0x0f) << 4)
 
+/* Divisor Latch Mode bits (EXAR extension)
+ *
+ * EXAR hardware is mostly compatible with SC16IS7XX, but supports additional feature:
+ * 4x and 8x divisor, instead of default 16x. It has a special register to program it.
+ * Bits 0 to 3 is fractional divisor, it used to set value of last 16 bits of
+ * uartclk * (16 / divisor) / baud, in case of default it will be uartclk / baud.
+ * Bits 4 and 5 used as switches, and should not be set to 1 simultaneously.
+ */
+
+#define SC16IS7XX_DLD_16X		0
+#define SC16IS7XX_DLD_DIV(m)	((m) & 0xf)
+#define SC16IS7XX_DLD_8X		BIT(4)
+#define SC16IS7XX_DLD_4X		BIT(5)
+
 /*
  * TLR register bits
  * If TLR[3:0] or TLR[7:4] are logical 0, the selectable trigger levels via the
@@ -310,6 +325,7 @@ struct sc16is7xx_devtype {
 	char	name[10];
 	int	nr_gpio;
 	int	nr_uart;
+	bool has_dld;
 };
 
 #define SC16IS7XX_RECONF_MD		(1 << 0)
@@ -522,6 +538,13 @@ static const struct sc16is7xx_devtype sc16is762_devtype = {
 	.nr_uart	= 2,
 };
 
+static const struct sc16is7xx_devtype xr20m1172_devtype = {
+	.name		= "XR20M1172",
+	.nr_gpio	= 8,
+	.nr_uart	= 2,
+	.has_dld	= true,
+};
+
 static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -559,13 +582,29 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
 	u8 lcr;
 	u8 prescaler = 0;
-	unsigned long clk = port->uartclk, div = clk / 16 / baud;
+	u8 divisor = 16;
+	u8 dld_mode = SC16IS7XX_DLD_16X;
+	bool has_dld = !!(port->flags & UPF_MAGIC_MULTIPLIER);
+	unsigned long clk = port->uartclk, div, div16;
+
+	if (has_dld)
+		while (DIV_ROUND_CLOSEST(port->uartclk, baud) < divisor)
+			divisor /= 2;
+
+	div16 = clk * (16 / divisor) / baud;
+	div = div16 / 16; /* For divisor = 16, it is the same as clk / 16 / baud */
 
 	if (div >= BIT(16)) {
 		prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
 		div /= 4;
 	}
 
+	/* Count additional divisor for EXAR devices */
+	if (divisor == 8)
+		dld_mode = SC16IS7XX_DLD_8X;
+	if (divisor == 4)
+		dld_mode = SC16IS7XX_DLD_4X;
+
 	/* Enable enhanced features */
 	sc16is7xx_efr_lock(port);
 	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
@@ -586,12 +625,14 @@ static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 	regcache_cache_bypass(one->regmap, true);
 	sc16is7xx_port_write(port, SC16IS7XX_DLH_REG, div / 256);
 	sc16is7xx_port_write(port, SC16IS7XX_DLL_REG, div % 256);
+	if (has_dld)
+		sc16is7xx_port_write(port, SC16IS7XX_DLD_REG, dld_mode | SC16IS7XX_DLD_DIV(div16));
 	regcache_cache_bypass(one->regmap, false);
 
 	/* Restore LCR and access to general register set */
 	sc16is7xx_port_write(port, SC16IS7XX_LCR_REG, lcr);
 
-	return DIV_ROUND_CLOSEST(clk / 16, div);
+	return DIV_ROUND_CLOSEST(clk / divisor, div);
 }
 
 static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
@@ -1014,6 +1055,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	unsigned int lcr, flow = 0;
 	int baud;
 	unsigned long flags;
+	bool has_dld = !!(port->flags & UPF_MAGIC_MULTIPLIER);
 
 	kthread_cancel_delayed_work_sync(&one->ms_work);
 
@@ -1093,7 +1135,7 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 	/* Get baud rate generator configuration */
 	baud = uart_get_baud_rate(port, termios, old,
 				  port->uartclk / 16 / 4 / 0xffff,
-				  port->uartclk / 16);
+				  port->uartclk / (has_dld ? 4 : 16));
 
 	/* Setup baudrate generator */
 	baud = sc16is7xx_set_baud(port, baud);
@@ -1550,6 +1592,9 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.type	= PORT_SC16IS7XX;
 		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
+		/* If we have DLD register, then set UPF_MAGIC_MULTIPLIER flag */
+		if (devtype->has_dld)
+			s->p[i].port.flags |= UPF_MAGIC_MULTIPLIER;
 		s->p[i].port.iobase	= i;
 		/*
 		 * Use all ones as membase to make sure uart_configure_port() in
@@ -1688,6 +1733,7 @@ static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
 	{ .compatible = "nxp,sc16is752",	.data = &sc16is752_devtype, },
 	{ .compatible = "nxp,sc16is760",	.data = &sc16is760_devtype, },
 	{ .compatible = "nxp,sc16is762",	.data = &sc16is762_devtype, },
+	{ .compatible = "exar,xr20m1172",	.data = &xr20m1172_devtype, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sc16is7xx_dt_ids);
@@ -1776,6 +1822,7 @@ static const struct spi_device_id sc16is7xx_spi_id_table[] = {
 	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
 	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
 	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },
 	{ }
 };
 
@@ -1826,6 +1873,7 @@ static const struct i2c_device_id sc16is7xx_i2c_id_table[] = {
 	{ "sc16is752",	(kernel_ulong_t)&sc16is752_devtype, },
 	{ "sc16is760",	(kernel_ulong_t)&sc16is760_devtype, },
 	{ "sc16is762",	(kernel_ulong_t)&sc16is762_devtype, },
+	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sc16is7xx_i2c_id_table);
-- 
2.34.1

Re: [PATCH v2 1/3] serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 04:57:32PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> Hardware supports both modes, but after

The hardware

"both"? Please elaborate, like

"The hardware supports both (A and B) modes, ..."

> commit 4afeced55baa ("serial: core: fix sanitizing check for RTS settings")

the commit

> we always end up with SER_RS485_RTS_AFTER_SEND and always write in

write to

> register SC16IS7XX_EFCR_RTS_INVERT_BIT,

the register

> which breaks some hardware using these chips,

Please, rewrap these lines to be approximately same width. Yes, you may split
commit over the lines in the body of the commit message, but tags (like Fixes).

Also it seems you forgot to add Vladimir's tags. It's your responsibility
to do that if you send a new version with only cosmetic changes done.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] serial: sc16is7xx: Add bindings documentation for EXAR XR20M1172 UART

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 04:57:33PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> This patch adds the devicetree documentation for the XR20M1172 UART.

Submitting Patches document says something about "imperative mood", please
read that document in full anyway as it has a lot of nice requirements and
recommendations.

...

>        - nxp,sc16is752
>        - nxp,sc16is760
>        - nxp,sc16is762
> +      - exar,xr20m1172

Wouldn't it be better to keep it alphabetically ordered (at least avoid
bringing more chaos if it's not ordered originally)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 04:57:34PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> Its register set is mostly compatible with SC16IS762, but

"Its"? Whose? Elaborate, please.

> it has a support for additional division rates of UART
> with special DLD register. So, add handling this register
> via UPF_MAGIC_MULTIPLIER port flag.

Oh, can we avoid using this? You can redefine ->set_termios() if required and
before factor out the common pieces to the helper functions.

...

All three commit messages seems follow different text width, please keep it around

~60 for Subject and ~72 for the commit message.

...

>  /* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
>  #define SC16IS7XX_DLL_REG		(0x00) /* Divisor Latch Low */
>  #define SC16IS7XX_DLH_REG		(0x01) /* Divisor Latch High */
> +#define SC16IS7XX_DLD_REG		(0x02) /* Divisor Latch Mode (only on EXAR chips) */

Is it called DLD in the datasheet? If so, can you match the comment to
the datasheet, otherwise make it DLM and I would even go for the EXAR
namespace.

...

> +/* Divisor Latch Mode bits (EXAR extension)
> + *
> + * EXAR hardware is mostly compatible with SC16IS7XX, but supports additional feature:
> + * 4x and 8x divisor, instead of default 16x. It has a special register to program it.
> + * Bits 0 to 3 is fractional divisor, it used to set value of last 16 bits of
> + * uartclk * (16 / divisor) / baud, in case of default it will be uartclk / baud.
> + * Bits 4 and 5 used as switches, and should not be set to 1 simultaneously.
> + */

/*
 * This is wrong multi-line comment
 * style for this subsystem. Use this
 * example.
 */

...

> +#define SC16IS7XX_DLD_16X		0
> +#define SC16IS7XX_DLD_DIV(m)	((m) & 0xf)

GENMASK() (since you already use BIT() below)

> +#define SC16IS7XX_DLD_8X		BIT(4)
> +#define SC16IS7XX_DLD_4X		BIT(5)

Perhaps also EXAR namespace.

...

>  	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
>  	u8 lcr;
>  	u8 prescaler = 0;
> -	unsigned long clk = port->uartclk, div = clk / 16 / baud;
> +	u8 divisor = 16;
> +	u8 dld_mode = SC16IS7XX_DLD_16X;
> +	bool has_dld = !!(port->flags & UPF_MAGIC_MULTIPLIER);
> +	unsigned long clk = port->uartclk, div, div16;

Please, try to keep it in reversed xmas tree order (longer lines first).

...

> +	if (has_dld)

Can we actually replace this with some ID checks?

> +		while (DIV_ROUND_CLOSEST(port->uartclk, baud) < divisor)
> +			divisor /= 2;

Bit shifts and ffs() / fls() from bitops.h (or respective round*() / ilog2() /
etc. from log2.h) will help you to avoid while loop.

...

> +	div16 = clk * (16 / divisor) / baud;
> +	div = div16 / 16; /* For divisor = 16, it is the same as clk / 16 / baud */


So, these may loose in precision, right?

Wouldn't be better to have

	div16 = (clk * 16) / divisor / baud;
	div = div16 / 16;

?

>  	if (div >= BIT(16)) {
>  		prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
>  		div /= 4;
>  	}
>  

...

>  	{ .compatible = "nxp,sc16is752",	.data = &sc16is752_devtype, },
>  	{ .compatible = "nxp,sc16is760",	.data = &sc16is760_devtype, },
>  	{ .compatible = "nxp,sc16is762",	.data = &sc16is762_devtype, },
> +	{ .compatible = "exar,xr20m1172",	.data = &xr20m1172_devtype, },

Sorted?

...

> +	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },

This gives a hint about the above mentioned EXAR namespace for the definitions,
i.e. use

#define XR20M1172_...

-- 
With Best Regards,
Andy Shevchenko