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

[PATCH v3 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.
Yes, in datasheet this register is called "DLD - Divisor Fractional"
or "DLD - Divisor Fractional Register", calling depends on datasheet
version.

Changes in v2:
  - use full name in git authorship

Changes in v3:
  - change formatting of commit messages to unify width
  - rework commit messages according to code review
  - add XR20M117X namespace for EXAR-specific register
  - do not use UPF_MAGIC_MULTIPLIER for checking EXAR chip,
    use s->devtype directly
  - replace while loop to fls function and expanded check
  - sort compatibles
  - reformat multiline comment.

Konstantin Pugin (3):
  serial: sc16is7xx: announce support of SER_RS485_RTS_ON_SEND
  dt-bindings: sc16is7xx: Add compatible line for XR20M1172 UART
  serial: sc16is7xx: add support for EXAR XR20M1172 UART

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

-- 
2.34.1

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

[Konstantin Pugin]

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

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

Fixes: 267913ecf737 ("serial: sc16is7xx: Fill in rs485_supported")
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
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

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

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 08:06:05PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> The hardware supports both RTS_ON_SEND and RTS_AFTER_SEND modes, but
> after the commit 4afeced55baa ("serial: core: fix sanitizing check for
> RTS settings") we always end up with SER_RS485_RTS_AFTER_SEND and

  "...with _RTS_AFTER_SEND set..."

or

  "...with _RTS_AFTER_SEND clear..."

or?..

> always write in the register field SC16IS7XX_EFCR_RTS_INVERT_BIT, which

write to

> breaks some hardware using these chips.

...

I might have been not clear about Vladimir's tag. Please double check
if he gave it against the certain patch or the entire series.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 08:52:36PM +0300, Konstantin P. wrote:

Please, do not top post!

> How I should check this? Vladimir does not said anything about his tag
> scope - whether it applies only to patch 2 or to series as a whole, and
> initially I assumed than his tag were given only to patch 2.

If there was not explicitly said, the algo is following:
- if the tag against cover letter — all patches are covered
- otherwise only the patches reply to which has been sent

> But then you said than I missed his tag, so, I thought that it applies to
> series as a whole and in version 3 I added his tag to all patches.

I might missed the difference explained above. Sorry about that.

> On Thu, Apr 18, 2024, 20:37 Andy Shevchenko <
> andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Thu, Apr 18, 2024 at 08:06:05PM +0300, Konstantin Pugin wrote:
> > > From: Konstantin Pugin <ria.freelander@gmail.com>

...

> > I might have been not clear about Vladimir's tag. Please double check
> > if he gave it against the certain patch or the entire series.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/3] dt-bindings: sc16is7xx: Add compatible line for XR20M1172 UART

[Konstantin Pugin]

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

Add EXAR XR20M1172 UART compatible line into devicetree documentation.

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..c4bedf23368b 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     enum:
+      - exar,xr20m1172
       - nxp,sc16is740
       - nxp,sc16is741
       - nxp,sc16is750
-- 
2.34.1

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

[Konstantin Pugin]

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

XR20M1172 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 by appropriate devicetree bindings.

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 55 +++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a300eebf1401..59376c637467 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 XR20M117X_DLD_REG		(0x02) /* Divisor Fractional Register (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 Fractional Register 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 XR20M117X_DLD_16X		0
+#define XR20M117X_DLD_DIV(m)	((m) & GENMASK(3, 0))
+#define XR20M117X_DLD_8X		BIT(4)
+#define XR20M117X_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) {
@@ -556,16 +579,33 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
 
 static int sc16is7xx_set_baud(struct uart_port *port, int baud)
 {
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
-	u8 lcr;
+	unsigned long clk = port->uartclk, div, div16;
+	bool has_dld = s->devtype->has_dld;
+	u8 dld_mode = XR20M117X_DLD_16X;
 	u8 prescaler = 0;
-	unsigned long clk = port->uartclk, div = clk / 16 / baud;
+	u8 divisor = 16;
+	u8 lcr;
+
+	if (has_dld && DIV_ROUND_CLOSEST(clk, baud) < 16)
+		divisor = 1 << (fls(DIV_ROUND_CLOSEST(clk, baud)) - 1);
+
+	div16 = (clk * 16) / divisor / baud;
+	div = div16 / 16;
 
 	if (div >= BIT(16)) {
 		prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
 		div /= 4;
 	}
 
+	/* Count additional divisor for EXAR devices */
+	if (divisor == 8)
+		dld_mode = XR20M117X_DLD_8X;
+	if (divisor == 4)
+		dld_mode = XR20M117X_DLD_4X;
+	dld_mode |= XR20M117X_DLD_DIV(div16);
+
 	/* Enable enhanced features */
 	sc16is7xx_efr_lock(port);
 	sc16is7xx_port_update(port, SC16IS7XX_EFR_REG,
@@ -586,12 +626,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, XR20M117X_DLD_REG, dld_mode);
 	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,
@@ -1010,7 +1052,9 @@ static void sc16is7xx_set_termios(struct uart_port *port,
 				  struct ktermios *termios,
 				  const struct ktermios *old)
 {
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+	bool has_dld = s->devtype->has_dld;
 	unsigned int lcr, flow = 0;
 	int baud;
 	unsigned long flags;
@@ -1093,7 +1137,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);
@@ -1682,6 +1726,7 @@ static void sc16is7xx_remove(struct device *dev)
 }
 
 static const struct of_device_id __maybe_unused sc16is7xx_dt_ids[] = {
+	{ .compatible = "exar,xr20m1172",	.data = &xr20m1172_devtype, },
 	{ .compatible = "nxp,sc16is740",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is741",	.data = &sc16is74x_devtype, },
 	{ .compatible = "nxp,sc16is750",	.data = &sc16is750_devtype, },
@@ -1776,6 +1821,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 +1872,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 v3 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 08:06:07PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> XR20M1172 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 by appropriate devicetree bindings.

...

>  /* 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 XR20M117X_DLD_REG		(0x02) /* Divisor Fractional Register (only on EXAR chips) */

The comment in the parentheses is not needed anymore as it's implied by
the namespace.

...

> +#define XR20M117X_DLD_16X		0
> +#define XR20M117X_DLD_DIV(m)	((m) & GENMASK(3, 0))

Seems like one too little TABs in between.

> +#define XR20M117X_DLD_8X		BIT(4)
> +#define XR20M117X_DLD_4X		BIT(5)

...

>  	char	name[10];
>  	int	nr_gpio;
>  	int	nr_uart;

> +	bool has_dld;

Not needed. See below.

...

> +	bool has_dld = s->devtype->has_dld;

So, you can check against devtype itself:

	s->devtype == &xr20m1172_devtype;

> +static const struct sc16is7xx_devtype = {

...

> +	if (has_dld && DIV_ROUND_CLOSEST(clk, baud) < 16)
> +		divisor = 1 << (fls(DIV_ROUND_CLOSEST(clk, baud)) - 1);

BIT() ?

-- 
With Best Regards,
Andy Shevchenko

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

[Jiri Slaby]

On 18. 04. 24, 19:06, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> XR20M1172 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 by appropriate devicetree bindings.
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 55 +++++++++++++++++++++++++++++++---
>   1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a300eebf1401..59376c637467 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
...
> @@ -556,16 +579,33 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg)
>   
>   static int sc16is7xx_set_baud(struct uart_port *port, int baud)
>   {
> +	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
>   	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
> -	u8 lcr;
> +	unsigned long clk = port->uartclk, div, div16;
> +	bool has_dld = s->devtype->has_dld;
> +	u8 dld_mode = XR20M117X_DLD_16X;
>   	u8 prescaler = 0;
> -	unsigned long clk = port->uartclk, div = clk / 16 / baud;
> +	u8 divisor = 16;
> +	u8 lcr;
> +
> +	if (has_dld && DIV_ROUND_CLOSEST(clk, baud) < 16)
> +		divisor = 1 << (fls(DIV_ROUND_CLOSEST(clk, baud)) - 1);

Do you mimic roundup_pow_of_two()?

-- 
js
suse labs

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

[Jiri Slaby]

On 18. 04. 24, 19:06, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> XR20M1172 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 by appropriate devicetree bindings.
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 55 +++++++++++++++++++++++++++++++---
>   1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a300eebf1401..59376c637467 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
...
> @@ -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 Fractional Register 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 XR20M117X_DLD_16X		0
> +#define XR20M117X_DLD_DIV(m)	((m) & GENMASK(3, 0))

Again, why not set this up as a mask and use FIELD_PREP?

Could you stop submitting this many series in such a short time lapse? 
It makes reviewing a major PITA.

-- 
js
suse labs

Re: [PATCH v3 0/3] add support for EXAR XR20M1172 UART

[Andy Shevchenko]

On Thu, Apr 18, 2024 at 08:06:04PM +0300, Konstantin Pugin wrote:
> 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.

> Yes, in datasheet this register is called "DLD - Divisor Fractional"
> or "DLD - Divisor Fractional Register", calling depends on datasheet
> version.

I see, interesting inconsistency, but that's what we have to live with.

...

I just have noticed that you haven't used the updated base for your series,
you need to take Greg's KH tty tree and use tty-next / tty-testing branch(es).

Speaking of base, start using --base when preparing patches, it helps CIs to
test against correct branches.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 0/3] add support for EXAR XR20M1172 UART

[Maarten Brock]

> From: Konstantin Pugin <rilian.la.te@ya.ru>
> Subject: [PATCH v3 0/3] add support for EXAR XR20M1172 UART

What is the policy in the kernel sources for the name of the manufacturer?
This driver never had special support for the EXAR chips when it was still EXAR.
Since 2017 it is now part of MaxLinear. Should the driver use the name of the
original manufacturer or the name of the manufacturer at the time of addition
to the sources?

Kind regards,
Maarten