Re: [RFC PATCH v2 2/2] serial: sc16is7xx: fix regression with GPIO configuration

From: Hugo Villeneuve
Date: Fri May 19 2023 - 09:41:51 EST

On Fri, 19 May 2023 15:00:58 +0200
Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx> wrote:

> Hello Hugo,
>
> A couple of remarks inline.
>
> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> >
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> >
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> >
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> >
> > This new patch allows to specify GPIO or modem control line function
> > in the device tree, and for each of the ports (A or B).
> >
> > This is done by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> >
> > Boards that need to have GPIOS configured as modem control lines
> > should add that property to their device tree. Here is a list of
> > boards using the sc16is7xx driver in their device tree and that may
> > need to be modified:
> > arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> > mips/boot/dts/ingenic/cu1830-neo.dts
> > mips/boot/dts/ingenic/cu1000-neo.dts
> >
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > ---
> > v1 -> v2: Rebase because original patch did not apply properly
> > Add git base-commit
> >
> > drivers/tty/serial/sc16is7xx.c | 42 ++++++++++++++++++++--------------
> > 1 file changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index abad091baeea..4e3d2325ef6e 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >
> > /* IOControl register bits (Only 750/760) */
> > #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */
> > -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */
> > #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */
> >
> > /* EFCR register bits */
> > @@ -306,7 +307,6 @@ struct sc16is7xx_devtype {
> > char name[10];
> > int nr_gpio;
> > int nr_uart;
> > - int has_mctrl;
> > };
> >
> > #define SC16IS7XX_RECONF_MD (1 << 0)
> > @@ -447,35 +447,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> > .name = "SC16IS74X",
> > .nr_gpio = 0,
> > .nr_uart = 1,
> > - .has_mctrl = 0,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is750_devtype = {
> > .name = "SC16IS750",
> > - .nr_gpio = 4,
> > + .nr_gpio = 8,
> > .nr_uart = 1,
> > - .has_mctrl = 1,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is752_devtype = {
> > .name = "SC16IS752",
> > - .nr_gpio = 0,
> > + .nr_gpio = 8,
> > .nr_uart = 2,
> > - .has_mctrl = 1,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is760_devtype = {
> > .name = "SC16IS760",
> > - .nr_gpio = 4,
> > + .nr_gpio = 8,
> > .nr_uart = 1,
> > - .has_mctrl = 1,
> > };
> >
> > static const struct sc16is7xx_devtype sc16is762_devtype = {
> > .name = "SC16IS762",
> > - .nr_gpio = 0,
> > + .nr_gpio = 8,
> > .nr_uart = 2,
> > - .has_mctrl = 1,
> > };
> >
> > static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> > @@ -1456,12 +1451,6 @@ static int sc16is7xx_probe(struct device *dev,
> > SC16IS7XX_EFCR_RXDISABLE_BIT |
> > SC16IS7XX_EFCR_TXDISABLE_BIT);
> >
> > - /* Use GPIO lines as modem status registers */
> > - if (devtype->has_mctrl)
> > - sc16is7xx_port_write(&s->p[i].port,
> > - SC16IS7XX_IOCONTROL_REG,
> > - SC16IS7XX_IOCONTROL_MODEM_BIT);
> > -
> > /* Initialize kthread work structs */
> > kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
> > kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
> > @@ -1497,6 +1486,25 @@ static int sc16is7xx_probe(struct device *dev,
> > prop, p, u)
> > if (u < devtype->nr_uart)
> > s->p[u].irda_mode = true;
> > +
> > + val = 0;
> > +
> > + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > + prop, p, u)
> The general idea looks solid to me.
> I think, that with this property set, we should also reduce number of GPIOs exported by gpiochip.
> > + if (u < devtype->nr_uart) {
> > + /* Use GPIO lines as modem control lines */
> > + if (u == 0)
> > + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > + else if (u == 1)
> > + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > + }
> > +
> > + if (val)
> > + regmap_update_bits(
> > + s->regmap,
> > + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > + SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > + SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> I just had a chance to test this on my hardware (with SC16IS760), and something still doesn't work.
> When looking into /sys/kernel/debug/regmap/<device>/registers,
> it seems like the proper bits are set, also taking the regmap shift into account.
> For reference - I did my test basing on v6.4-rc2.
>
> I used a loopback plug and statserial to perform the test.
> Without your patch, when DTR is set, all other pins (DCD, DSR, RI) are set. With it - they are cleared, according to statserial.

Maybe it is not related, but I have submitted a separate patch to fix an initialization bug with port 0, maybe you can try also with this patch first applied:

https://lkml.org/lkml/2023/5/17/1311

Hugo.


> > }
> >
> > #ifdef CONFIG_GPIOLIB
> > --
> > 2.30.2
> >
> >
> --
> Pozdrawiam/With kind regards,
> Lech Perczak
>
> Sr. Software Engineer
> Camlin Technologies Poland Limited Sp. z o.o.
> Strzegomska 54,
> 53-611 Wroclaw
> Tel: (+48) 71 75 000 16
> Email: lech.perczak@xxxxxxxxxxxxxxx
> Website: http://www.camlingroup.com
>