| Message ID | 20240418170610.759838-1-rilian.la.te@ya.ru |
|---|---|
| Headers | show |
| Series | add support for EXAR XR20M1172 UART | expand |
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.
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() ?
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.
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.
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()?
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.
> 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
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(-)