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

[Hugo Villeneuve]

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.

OK, I will look into that.

> > +                       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.

It is strange, because the only conceptual change is the moment in time when we set the IOControl register. Before, IOControl was set within the for/loop for initializing each port. Now it is done after.

Maybe you could try to put the section that reads the DT property just after resetting the device.

While suggesting this, I just noticed that when the device is reset, we do not wait before initiating communication with the device. Thefore, I would suggest the following change (that I will submit as a separate patch):

        regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
                        SC16IS7XX_IOCONTROL_SRESET_BIT);
 
+       /* After reset, the host must wait at least 3us before initializing a
+        * communication with the device: */
+       usleep_range(3, 5);

Then like I said, insert the code that read the modem-control-line-port DT property right after that to see if it helps.

Hugo.