| Message ID | 20240424191908.32565-1-rilian.la.te@ya.ru |
|---|---|
| Headers | show |
| Series | add support for EXAR XR20M1172 UART | expand |
On Wed, Apr 24, 2024 at 10:18:51PM +0300, Konstantin Pugin wrote: > 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. A side note about email. It seems you have To: email header empty which may increase chances to get these messages into spam. Again, I recommend to look at the implementation of my script [1] which I use on daily-basis and re-use it or taking an ideas how to send patches to the Linux kernel mailing lists. Alternatively you may study `b4` tool, it also has something like that in recent versions. [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
On Wed, 24 Apr 2024 22:18:54 +0300 Konstantin Pugin <rilian.la.te@ya.ru> wrote: Hi Konstantin, > 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. -> "... but it supports additional division rates with special..." > So, add handling this register by appropriate devicetree bindings. I am not sure about this, support for DLD is added in the driver, not in device tree. You can probably drop that sentence? > > Reviewed-by: Andy Shevchenko <andy@kernel.org> > Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com> > --- > drivers/tty/serial/Kconfig | 3 +- > drivers/tty/serial/sc16is7xx.c | 61 ++++++++++++++++++++++++++++-- > drivers/tty/serial/sc16is7xx.h | 1 + > drivers/tty/serial/sc16is7xx_i2c.c | 1 + > drivers/tty/serial/sc16is7xx_spi.c | 1 + > 5 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 4fdd7857ef4d..9d0438cfe147 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1029,7 +1029,7 @@ config SERIAL_SC16IS7XX_CORE > select SERIAL_SC16IS7XX_SPI if SPI_MASTER > select SERIAL_SC16IS7XX_I2C if I2C > help > - Core driver for NXP SC16IS7xx UARTs. > + Core driver for NXP SC16IS7xx and compatible UARTs. > Supported ICs are: > > SC16IS740 > @@ -1038,6 +1038,7 @@ config SERIAL_SC16IS7XX_CORE > SC16IS752 > SC16IS760 > SC16IS762 > + XR20M1172 (Exar) > > The driver supports both I2C and SPI interfaces. > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index dfcc804f558f..09c9e52d7ec2 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -10,6 +10,7 @@ > #undef DEFAULT_SYMBOL_NAMESPACE > #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX > > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/device.h> > @@ -68,6 +69,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 */ Remove "Register". Also, DLD register needs EFR[4] = 1 to be accessed, so maybe add this to the comment: /* Divisor Fractional (requires EFR[4] = 1) */ > > /* Enhanced Register set: Only if (LCR == 0xBF) */ > #define SC16IS7XX_EFR_REG (0x02) /* Enhanced Features */ > @@ -221,6 +223,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_MASK 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 > @@ -523,6 +539,13 @@ const struct sc16is7xx_devtype sc16is762_devtype = { > }; > EXPORT_SYMBOL_GPL(sc16is762_devtype); > > +const struct sc16is7xx_devtype xr20m1172_devtype = { > + .name = "XR20M1172", > + .nr_gpio = 8, > + .nr_uart = 2, > +}; > +EXPORT_SYMBOL_GPL(xr20m1172_devtype); > + > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > { > switch (reg) { > @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg) > return reg == SC16IS7XX_RHR_REG; > } > > +static bool sc16is7xx_has_dld(struct device *dev) > +{ > + struct sc16is7xx_port *s = dev_get_drvdata(dev); > + > + if (s->devtype == &xr20m1172_devtype) > + return true; > + return false; > +} > + > static int sc16is7xx_set_baud(struct uart_port *port, int baud) > { > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > - u8 lcr; > + unsigned long clk = port->uartclk, div, div16; > + bool has_dld = sc16is7xx_has_dld(port->dev); > + 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 = rounddown_pow_of_two(DIV_ROUND_CLOSEST(clk, baud)); > + > + 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 |= FIELD_PREP(XR20M117X_DLD_DIV_MASK, div16); > + > /* Enable enhanced features */ > sc16is7xx_efr_lock(port); > sc16is7xx_port_update(port, SC16IS7XX_EFR_REG, > @@ -587,12 +635,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, > @@ -1002,6 +1052,8 @@ static void sc16is7xx_set_termios(struct uart_port *port, > const struct ktermios *old) > { > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > + bool has_dld = sc16is7xx_has_dld(port->dev); > + u8 divisor = has_dld ? 4 : 16 This still breaks compilation. I noted in V7 of the patch to fix this, but obviously you didn't do it, altough you mention that you would do it in your reply to me, and you also mention doing it in your cover letter... This means you didn't test your patchset V8 before sending it. You really need to properly test _before_ sending a new patchset. Hugo. > unsigned int lcr, flow = 0; > int baud; > unsigned long flags; > @@ -1084,7 +1136,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 / divisor); > > /* Setup baudrate generator */ > baud = sc16is7xx_set_baud(port, baud); > @@ -1684,6 +1736,7 @@ void sc16is7xx_remove(struct device *dev) > EXPORT_SYMBOL_GPL(sc16is7xx_remove); > > 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, }, > diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h > index afb784eaee45..eb2e3bc86f15 100644 > --- a/drivers/tty/serial/sc16is7xx.h > +++ b/drivers/tty/serial/sc16is7xx.h > @@ -28,6 +28,7 @@ extern const struct sc16is7xx_devtype sc16is750_devtype; > extern const struct sc16is7xx_devtype sc16is752_devtype; > extern const struct sc16is7xx_devtype sc16is760_devtype; > extern const struct sc16is7xx_devtype sc16is762_devtype; > +extern const struct sc16is7xx_devtype xr20m1172_devtype; > > const char *sc16is7xx_regmap_name(u8 port_id); > > diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c > index 3ed47c306d85..839de902821b 100644 > --- a/drivers/tty/serial/sc16is7xx_i2c.c > +++ b/drivers/tty/serial/sc16is7xx_i2c.c > @@ -46,6 +46,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); > diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c > index 73df36f8a7fd..2b278282dbd0 100644 > --- a/drivers/tty/serial/sc16is7xx_spi.c > +++ b/drivers/tty/serial/sc16is7xx_spi.c > @@ -69,6 +69,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, }, > { } > }; > MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table); > -- > 2.44.0 > > >
On Thu, Apr 25, 2024 at 6:26 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > On Wed, 24 Apr 2024 22:18:54 +0300 > Konstantin Pugin <rilian.la.te@ya.ru> wrote: > > Hi Konstantin, > > > 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. > > -> "... but it supports additional division rates with special..." > > > So, add handling this register by appropriate devicetree bindings. > > I am not sure about this, support for DLD is added in the driver, > not in device tree. You can probably drop that sentence? > > > > > Reviewed-by: Andy Shevchenko <andy@kernel.org> > > Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com> > > --- > > drivers/tty/serial/Kconfig | 3 +- > > drivers/tty/serial/sc16is7xx.c | 61 ++++++++++++++++++++++++++++-- > > drivers/tty/serial/sc16is7xx.h | 1 + > > drivers/tty/serial/sc16is7xx_i2c.c | 1 + > > drivers/tty/serial/sc16is7xx_spi.c | 1 + > > 5 files changed, 62 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > index 4fdd7857ef4d..9d0438cfe147 100644 > > --- a/drivers/tty/serial/Kconfig > > +++ b/drivers/tty/serial/Kconfig > > @@ -1029,7 +1029,7 @@ config SERIAL_SC16IS7XX_CORE > > select SERIAL_SC16IS7XX_SPI if SPI_MASTER > > select SERIAL_SC16IS7XX_I2C if I2C > > help > > - Core driver for NXP SC16IS7xx UARTs. > > + Core driver for NXP SC16IS7xx and compatible UARTs. > > Supported ICs are: > > > > SC16IS740 > > @@ -1038,6 +1038,7 @@ config SERIAL_SC16IS7XX_CORE > > SC16IS752 > > SC16IS760 > > SC16IS762 > > + XR20M1172 (Exar) > > > > The driver supports both I2C and SPI interfaces. > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index dfcc804f558f..09c9e52d7ec2 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -10,6 +10,7 @@ > > #undef DEFAULT_SYMBOL_NAMESPACE > > #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX > > > > +#include <linux/bitfield.h> > > #include <linux/clk.h> > > #include <linux/delay.h> > > #include <linux/device.h> > > @@ -68,6 +69,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 */ > > Remove "Register". > > Also, DLD register needs EFR[4] = 1 to be accessed, so maybe add this > to the comment: > > /* Divisor Fractional (requires EFR[4] = 1) */ > > > > > > /* Enhanced Register set: Only if (LCR == 0xBF) */ > > #define SC16IS7XX_EFR_REG (0x02) /* Enhanced Features */ > > @@ -221,6 +223,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_MASK 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 > > @@ -523,6 +539,13 @@ const struct sc16is7xx_devtype sc16is762_devtype = { > > }; > > EXPORT_SYMBOL_GPL(sc16is762_devtype); > > > > +const struct sc16is7xx_devtype xr20m1172_devtype = { > > + .name = "XR20M1172", > > + .nr_gpio = 8, > > + .nr_uart = 2, > > +}; > > +EXPORT_SYMBOL_GPL(xr20m1172_devtype); > > + > > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > > { > > switch (reg) { > > @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg) > > return reg == SC16IS7XX_RHR_REG; > > } > > > > +static bool sc16is7xx_has_dld(struct device *dev) > > +{ > > + struct sc16is7xx_port *s = dev_get_drvdata(dev); > > + > > + if (s->devtype == &xr20m1172_devtype) > > + return true; > > + return false; > > +} > > + > > static int sc16is7xx_set_baud(struct uart_port *port, int baud) > > { > > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > > - u8 lcr; > > + unsigned long clk = port->uartclk, div, div16; > > + bool has_dld = sc16is7xx_has_dld(port->dev); > > + 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 = rounddown_pow_of_two(DIV_ROUND_CLOSEST(clk, baud)); > > + > > + 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 |= FIELD_PREP(XR20M117X_DLD_DIV_MASK, div16); > > + > > /* Enable enhanced features */ > > sc16is7xx_efr_lock(port); > > sc16is7xx_port_update(port, SC16IS7XX_EFR_REG, > > @@ -587,12 +635,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, > > @@ -1002,6 +1052,8 @@ static void sc16is7xx_set_termios(struct uart_port *port, > > const struct ktermios *old) > > { > > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > > + bool has_dld = sc16is7xx_has_dld(port->dev); > > + u8 divisor = has_dld ? 4 : 16 > > This still breaks compilation. I noted in V7 of the patch to fix this, > but obviously you didn't do it, altough you mention that you would do > it in your reply to me, and you also mention doing it in your cover > letter... > > This means you didn't test your patchset V8 before sending it. You > really need to properly test _before_ sending a new patchset. > > > Hugo. > > > > > > unsigned int lcr, flow = 0; > > int baud; > > unsigned long flags; > > @@ -1084,7 +1136,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 / divisor); > > > > /* Setup baudrate generator */ > > baud = sc16is7xx_set_baud(port, baud); > > @@ -1684,6 +1736,7 @@ void sc16is7xx_remove(struct device *dev) > > EXPORT_SYMBOL_GPL(sc16is7xx_remove); > > > > 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, }, > > diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h > > index afb784eaee45..eb2e3bc86f15 100644 > > --- a/drivers/tty/serial/sc16is7xx.h > > +++ b/drivers/tty/serial/sc16is7xx.h > > @@ -28,6 +28,7 @@ extern const struct sc16is7xx_devtype sc16is750_devtype; > > extern const struct sc16is7xx_devtype sc16is752_devtype; > > extern const struct sc16is7xx_devtype sc16is760_devtype; > > extern const struct sc16is7xx_devtype sc16is762_devtype; > > +extern const struct sc16is7xx_devtype xr20m1172_devtype; > > > > const char *sc16is7xx_regmap_name(u8 port_id); > > > > diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c > > index 3ed47c306d85..839de902821b 100644 > > --- a/drivers/tty/serial/sc16is7xx_i2c.c > > +++ b/drivers/tty/serial/sc16is7xx_i2c.c > > @@ -46,6 +46,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); > > diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c > > index 73df36f8a7fd..2b278282dbd0 100644 > > --- a/drivers/tty/serial/sc16is7xx_spi.c > > +++ b/drivers/tty/serial/sc16is7xx_spi.c > > @@ -69,6 +69,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, }, > > { } > > }; > > MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table); > > -- > > 2.44.0 > > > > > > > > > -- > Hugo Villeneuve I compiled a kernel before sending on x86. Which options are you using? I will try to replicate it. I cannot run this kernel on actual hardware, because I need vendor patches - support of my board is not mainlined.
On Thu, 25 Apr 2024 18:43:20 +0300 "Konstantin P." <ria.freelander@gmail.com> wrote: > On Thu, Apr 25, 2024 at 6:26 PM Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Wed, 24 Apr 2024 22:18:54 +0300 > > Konstantin Pugin <rilian.la.te@ya.ru> wrote: > > > > Hi Konstantin, > > > > > 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. > > > > -> "... but it supports additional division rates with special..." > > > > > So, add handling this register by appropriate devicetree bindings. > > > > I am not sure about this, support for DLD is added in the driver, > > not in device tree. You can probably drop that sentence? > > > > > > > > Reviewed-by: Andy Shevchenko <andy@kernel.org> > > > Signed-off-by: Konstantin Pugin <ria.freelander@gmail.com> > > > --- > > > drivers/tty/serial/Kconfig | 3 +- > > > drivers/tty/serial/sc16is7xx.c | 61 ++++++++++++++++++++++++++++-- > > > drivers/tty/serial/sc16is7xx.h | 1 + > > > drivers/tty/serial/sc16is7xx_i2c.c | 1 + > > > drivers/tty/serial/sc16is7xx_spi.c | 1 + > > > 5 files changed, 62 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > > > index 4fdd7857ef4d..9d0438cfe147 100644 > > > --- a/drivers/tty/serial/Kconfig > > > +++ b/drivers/tty/serial/Kconfig > > > @@ -1029,7 +1029,7 @@ config SERIAL_SC16IS7XX_CORE > > > select SERIAL_SC16IS7XX_SPI if SPI_MASTER > > > select SERIAL_SC16IS7XX_I2C if I2C > > > help > > > - Core driver for NXP SC16IS7xx UARTs. > > > + Core driver for NXP SC16IS7xx and compatible UARTs. > > > Supported ICs are: > > > > > > SC16IS740 > > > @@ -1038,6 +1038,7 @@ config SERIAL_SC16IS7XX_CORE > > > SC16IS752 > > > SC16IS760 > > > SC16IS762 > > > + XR20M1172 (Exar) > > > > > > The driver supports both I2C and SPI interfaces. > > > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > > index dfcc804f558f..09c9e52d7ec2 100644 > > > --- a/drivers/tty/serial/sc16is7xx.c > > > +++ b/drivers/tty/serial/sc16is7xx.c > > > @@ -10,6 +10,7 @@ > > > #undef DEFAULT_SYMBOL_NAMESPACE > > > #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX > > > > > > +#include <linux/bitfield.h> > > > #include <linux/clk.h> > > > #include <linux/delay.h> > > > #include <linux/device.h> > > > @@ -68,6 +69,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 */ > > > > Remove "Register". > > > > Also, DLD register needs EFR[4] = 1 to be accessed, so maybe add this > > to the comment: > > > > /* Divisor Fractional (requires EFR[4] = 1) */ > > > > > > > > > > /* Enhanced Register set: Only if (LCR == 0xBF) */ > > > #define SC16IS7XX_EFR_REG (0x02) /* Enhanced Features */ > > > @@ -221,6 +223,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_MASK 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 > > > @@ -523,6 +539,13 @@ const struct sc16is7xx_devtype sc16is762_devtype = { > > > }; > > > EXPORT_SYMBOL_GPL(sc16is762_devtype); > > > > > > +const struct sc16is7xx_devtype xr20m1172_devtype = { > > > + .name = "XR20M1172", > > > + .nr_gpio = 8, > > > + .nr_uart = 2, > > > +}; > > > +EXPORT_SYMBOL_GPL(xr20m1172_devtype); > > > + > > > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg) > > > { > > > switch (reg) { > > > @@ -555,18 +578,43 @@ static bool sc16is7xx_regmap_noinc(struct device *dev, unsigned int reg) > > > return reg == SC16IS7XX_RHR_REG; > > > } > > > > > > +static bool sc16is7xx_has_dld(struct device *dev) > > > +{ > > > + struct sc16is7xx_port *s = dev_get_drvdata(dev); > > > + > > > + if (s->devtype == &xr20m1172_devtype) > > > + return true; > > > + return false; > > > +} > > > + > > > static int sc16is7xx_set_baud(struct uart_port *port, int baud) > > > { > > > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > > > - u8 lcr; > > > + unsigned long clk = port->uartclk, div, div16; > > > + bool has_dld = sc16is7xx_has_dld(port->dev); > > > + 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 = rounddown_pow_of_two(DIV_ROUND_CLOSEST(clk, baud)); > > > + > > > + 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 |= FIELD_PREP(XR20M117X_DLD_DIV_MASK, div16); > > > + > > > /* Enable enhanced features */ > > > sc16is7xx_efr_lock(port); > > > sc16is7xx_port_update(port, SC16IS7XX_EFR_REG, > > > @@ -587,12 +635,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, > > > @@ -1002,6 +1052,8 @@ static void sc16is7xx_set_termios(struct uart_port *port, > > > const struct ktermios *old) > > > { > > > struct sc16is7xx_one *one = to_sc16is7xx_one(port, port); > > > + bool has_dld = sc16is7xx_has_dld(port->dev); > > > + u8 divisor = has_dld ? 4 : 16 > > > > This still breaks compilation. I noted in V7 of the patch to fix this, > > but obviously you didn't do it, altough you mention that you would do > > it in your reply to me, and you also mention doing it in your cover > > letter... > > > > This means you didn't test your patchset V8 before sending it. You > > really need to properly test _before_ sending a new patchset. > > > > > > Hugo. > > > > > > > > > > > unsigned int lcr, flow = 0; > > > int baud; > > > unsigned long flags; > > > @@ -1084,7 +1136,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 / divisor); > > > > > > /* Setup baudrate generator */ > > > baud = sc16is7xx_set_baud(port, baud); > > > @@ -1684,6 +1736,7 @@ void sc16is7xx_remove(struct device *dev) > > > EXPORT_SYMBOL_GPL(sc16is7xx_remove); > > > > > > 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, }, > > > diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h > > > index afb784eaee45..eb2e3bc86f15 100644 > > > --- a/drivers/tty/serial/sc16is7xx.h > > > +++ b/drivers/tty/serial/sc16is7xx.h > > > @@ -28,6 +28,7 @@ extern const struct sc16is7xx_devtype sc16is750_devtype; > > > extern const struct sc16is7xx_devtype sc16is752_devtype; > > > extern const struct sc16is7xx_devtype sc16is760_devtype; > > > extern const struct sc16is7xx_devtype sc16is762_devtype; > > > +extern const struct sc16is7xx_devtype xr20m1172_devtype; > > > > > > const char *sc16is7xx_regmap_name(u8 port_id); > > > > > > diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c > > > index 3ed47c306d85..839de902821b 100644 > > > --- a/drivers/tty/serial/sc16is7xx_i2c.c > > > +++ b/drivers/tty/serial/sc16is7xx_i2c.c > > > @@ -46,6 +46,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); > > > diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c > > > index 73df36f8a7fd..2b278282dbd0 100644 > > > --- a/drivers/tty/serial/sc16is7xx_spi.c > > > +++ b/drivers/tty/serial/sc16is7xx_spi.c > > > @@ -69,6 +69,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, }, > > > { } > > > }; > > > MODULE_DEVICE_TABLE(spi, sc16is7xx_spi_id_table); > > > -- > > > 2.44.0 > > > > > > > > > > > > > > > -- > > Hugo Villeneuve > > I compiled a kernel before sending on x86. Which options are you > using? I will try to replicate it. I cannot run this kernel on actual > hardware, because I need vendor patches - support of my board is not > mainlined. Hi, please reply in line. Also, in your configuration, did you even bother to check that the SC16 driver is selected? Looks like you did not... And why do you say in your cover letter that you fixed this if it is not the case? Hugo.
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 am sorry about too many submissions and top post reply. About second - I do not know how to reply properly to this ML from GMail phone app. About first - I just get very good feedback from Andy Shevchenko, and want to fix his review picks ASAP. 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. Changes in v4: - rebase onto tty-next branch - added Kconfig mention of the chip - used rounddown_power_of_two instead of fls and manual shift - used FIELD_PREP instead of custom macro - removed has_dld bit from common struct, replaced by check function, which checks directly by s->devtype - fixed tab count - properly apply Vladimir Zapolskiy's tag to patch 2 only Changes in v5: - fixes for tty-next branch - address a new code review picks - send properly to all participants - added Ack tag Changes in v6: - KConfig fixes - New code review fixes Changes in v7: - Added missed tag - Added missed v5 fixes Changes in v8: - Fixed semicolon - Added missed tags - Fixed commit messages 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/Kconfig | 3 +- drivers/tty/serial/sc16is7xx.c | 63 +++++++++++++++++-- drivers/tty/serial/sc16is7xx.h | 1 + drivers/tty/serial/sc16is7xx_i2c.c | 1 + drivers/tty/serial/sc16is7xx_spi.c | 1 + 6 files changed, 64 insertions(+), 6 deletions(-) base-commit: 660a708098569a66a47d0abdad998e29e1259de6