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

  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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@dimonoff.com>
---
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(-)
  

Hello Hugo,

A couple of remarks inline.

W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> 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@dimonoff.com>
> ---
> 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.
>         }
>
>  #ifdef CONFIG_GPIOLIB
> --
> 2.30.2
>
>
  

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

> Hello Hugo,
> 
> A couple of remarks inline.
> 
> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > 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@dimonoff.com>
> > ---
> > 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.
  

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

> Hello Hugo,
> 
> A couple of remarks inline.
> 
> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > 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@dimonoff.com>
> > ---
> > 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@camlingroup.com
> Website: http://www.camlingroup.com
>
  

W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
> On Fri, 19 May 2023 15:00:58 +0200
> Lech Perczak <lech.perczak@camlingroup.com> wrote:
>
>> Hello Hugo,
>>
>> A couple of remarks inline.
>>
>> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>
>>> 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@dimonoff.com>
>>> ---
>>> 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.

Hello,

I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
Maybe the offsets in regmap are still wrong, for some reason?
I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.
  

On Fri, 19 May 2023 18:00:54 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:

> 
> W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
> > On Fri, 19 May 2023 15:00:58 +0200
> > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> >
> >> Hello Hugo,
> >>
> >> A couple of remarks inline.
> >>
> >> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> >>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>
> >>> 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@dimonoff.com>
> >>> ---
> >>> 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.
> 
> Hello,
> 
> I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
> I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
> Maybe the offsets in regmap are still wrong, for some reason?
> I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.

Then it seems that the remaining difference is the number of GPIOs that are configured, which for the moment is always the value set in constant nr_gpio (for each device).

I already implemented and tested your suggestion of lowering the number of GPIOs exported when using some pins as modem status lines. It seems to be working good, altough I have only tested that the correct number of GPIOs are now exported.

I will send you the test patch and let me know if it helps.

Hugo.
  

W dniu 19.05.2023 o 18:38, Hugo Villeneuve pisze:
> On Fri, 19 May 2023 18:00:54 +0200
> Lech Perczak <lech.perczak@camlingroup.com> wrote:
>
>> W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
>>> On Fri, 19 May 2023 15:00:58 +0200
>>> Lech Perczak <lech.perczak@camlingroup.com> wrote:
>>>
>>>> Hello Hugo,
>>>>
>>>> A couple of remarks inline.
>>>>
>>>> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
>>>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>>>>
>>>>> 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@dimonoff.com>
>>>>> ---
>>>>> 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.
>> Hello,
>>
>> I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
>> I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
>> Maybe the offsets in regmap are still wrong, for some reason?
>> I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.
> Then it seems that the remaining difference is the number of GPIOs that are configured, which for the moment is always the value set in constant nr_gpio (for each device).
>
> I already implemented and tested your suggestion of lowering the number of GPIOs exported when using some pins as modem status lines. It seems to be working good, altough I have only tested that the correct number of GPIOs are now exported.
>
> I will send you the test patch and let me know if it helps.
>
> Hugo.
Hello Hugo,

I applied the patch marking IOControl register as volatile - and this one fixed the issue. Modem line states are now reported correctly.
I think it'll be a good idea to bundle all the fixes in the series, when they become ready for submission.
  

On Fri, 19 May 2023 18:56:00 +0200
Lech Perczak <lech.perczak@camlingroup.com> wrote:

> 
> W dniu 19.05.2023 o 18:38, Hugo Villeneuve pisze:
> > On Fri, 19 May 2023 18:00:54 +0200
> > Lech Perczak <lech.perczak@camlingroup.com> wrote:
> >
> >> W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
> >>> On Fri, 19 May 2023 15:00:58 +0200
> >>> Lech Perczak <lech.perczak@camlingroup.com> wrote:
> >>>
> >>>> Hello Hugo,
> >>>>
> >>>> A couple of remarks inline.
> >>>>
> >>>> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> >>>>> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >>>>>
> >>>>> 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@dimonoff.com>
> >>>>> ---
> >>>>> 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.
> >> Hello,
> >>
> >> I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
> >> I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
> >> Maybe the offsets in regmap are still wrong, for some reason?
> >> I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.
> > Then it seems that the remaining difference is the number of GPIOs that are configured, which for the moment is always the value set in constant nr_gpio (for each device).
> >
> > I already implemented and tested your suggestion of lowering the number of GPIOs exported when using some pins as modem status lines. It seems to be working good, altough I have only tested that the correct number of GPIOs are now exported.
> >
> > I will send you the test patch and let me know if it helps.
> >
> > Hugo.
> Hello Hugo,
> 
> I applied the patch marking IOControl register as volatile - and this one fixed the issue. Modem line states are now reported correctly.
> I think it'll be a good idea to bundle all the fixes in the series, when they become ready for submission.

Hi Lech,
glad to hear this.

I wanted to keep the patches separate, to not interfere with fixing the GPIO regression, but now I see that it caused way more headaches than anticipated :)

I will integrate all the sc16is7xx patches in a new serie and it should be easier to apply/test.

And let me know if the patch to reduce the number of exported GPIOs is working as expected for you.

Thank you,
Hugo.