[RFC PATCH v2 1/2] dt-bindings: sc16is7xx: Add property to change GPIO function

[RFC PATCH v2 1/2] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Some variants in this series of uart controllers have GPIO pins that
are shared between GPIO and modem control lines.

The pin mux mode (GPIO or modem control lines) can be set for each
ports (channels) supported by the variant.

This adds a property to the device tree to set the GPIO pin mux to
modem control lines on selected ports if needed.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
v1 -> v2: Add nxp vendor prefix
          Add git base-commit

 .../bindings/serial/nxp,sc16is7xx.txt         | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index 0fa8e3e43bf8..23a81c83f012 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -23,6 +23,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as modem
+				control lines.
 
 Example:
         sc16is750: sc16is750@51 {
@@ -35,6 +38,17 @@ Example:
                 #gpio-cells = <2>;
         };
 
+	sc16is752: sc16is752@54 {
+		compatible = "nxp,sc16is752";
+		reg = <0x54>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
 * spi as bus
 
 Required properties:
@@ -59,6 +73,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as modem
+				control lines.
 
 Example:
 	sc16is750: sc16is750@0 {
@@ -70,3 +87,14 @@ Example:
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	sc16is752: sc16is752@0 {
+		compatible = "nxp,sc16is752";
+		reg = <0>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};

base-commit: 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff
-- 
2.30.2

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

[Hugo Villeneuve]

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)
+			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);
 	}
 
 #ifdef CONFIG_GPIOLIB
-- 
2.30.2

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

[Lech Perczak]

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

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

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

[Lech Perczak]

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.

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

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

[Hugo Villeneuve]

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.

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

[Lech Perczak]

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.

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

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

[Hugo Villeneuve]

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.

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@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
> 

Re: [RFC PATCH v2 1/2] dt-bindings: sc16is7xx: Add property to change GPIO function

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

On Thu, May 18, 2023 at 09:29:06AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Some variants in this series of uart controllers have GPIO pins that
> are shared between GPIO and modem control lines.
> 
> The pin mux mode (GPIO or modem control lines) can be set for each
> ports (channels) supported by the variant.
> 
> This adds a property to the device tree to set the GPIO pin mux to
> modem control lines on selected ports if needed.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

On the basis of similarity to existing properties due to my lack of
understanding of the hardware it'll have to be an Ack. Hopefully Greg &
Co. have more specific insight.
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
> v1 -> v2: Add nxp vendor prefix
>           Add git base-commit
> 
>  .../bindings/serial/nxp,sc16is7xx.txt         | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index 0fa8e3e43bf8..23a81c83f012 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -23,6 +23,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>  		   should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +				should have shared GPIO lines configured as modem
> +				control lines.
>  
>  Example:
>          sc16is750: sc16is750@51 {
> @@ -35,6 +38,17 @@ Example:
>                  #gpio-cells = <2>;
>          };
>  
> +	sc16is752: sc16is752@54 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0x54>;
> +		clocks = <&clk20m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> +		gpio-controller; /* Port 0 as GPIOs */
> +		#gpio-cells = <2>;
> +	};
> +
>  * spi as bus
>  
>  Required properties:
> @@ -59,6 +73,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>  		   should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +				should have shared GPIO lines configured as modem
> +				control lines.
>  
>  Example:
>  	sc16is750: sc16is750@0 {
> @@ -70,3 +87,14 @@ Example:
>  		gpio-controller;
>  		#gpio-cells = <2>;
>  	};
> +
> +	sc16is752: sc16is752@0 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0>;
> +		clocks = <&clk20m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> +		gpio-controller; /* Port 0 as GPIOs */
> +		#gpio-cells = <2>;
> +	};
> 
> base-commit: 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH v2 1/2] dt-bindings: sc16is7xx: Add property to change GPIO function

[Lech Perczak]

Hello Hugo,

W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Some variants in this series of uart controllers have GPIO pins that
> are shared between GPIO and modem control lines.
>
> The pin mux mode (GPIO or modem control lines) can be set for each
> ports (channels) supported by the variant.
>
> This adds a property to the device tree to set the GPIO pin mux to
> modem control lines on selected ports if needed.
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> v1 -> v2: Add nxp vendor prefix
>           Add git base-commit
>
>  .../bindings/serial/nxp,sc16is7xx.txt         | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index 0fa8e3e43bf8..23a81c83f012 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -23,6 +23,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>                    should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +                               should have shared GPIO lines configured as modem
> +                               control lines.
>
>  Example:
>          sc16is750: sc16is750@51 {
> @@ -35,6 +38,17 @@ Example:
>                  #gpio-cells = <2>;
>          };
>
> +       sc16is752: sc16is752@54 {
> +               compatible = "nxp,sc16is752";
> +               reg = <0x54>;
> +               clocks = <&clk20m>;
> +               interrupt-parent = <&gpio3>;
> +               interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +               nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> +               gpio-controller; /* Port 0 as GPIOs */
> +               #gpio-cells = <2>;
> +       };
> +
>  * spi as bus
>
>  Required properties:
> @@ -59,6 +73,9 @@ Optional properties:
>      1 = active low.
>  - irda-mode-ports: An array that lists the indices of the port that
>                    should operate in IrDA mode.
> +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> +                               should have shared GPIO lines configured as modem
> +                               control lines.
>
>  Example:
>         sc16is750: sc16is750@0 {
> @@ -70,3 +87,14 @@ Example:
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
> +
> +       sc16is752: sc16is752@0 {
> +               compatible = "nxp,sc16is752";
> +               reg = <0>;
> +               clocks = <&clk20m>;
> +               interrupt-parent = <&gpio3>;
> +               interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> +               nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
Just a minor nitpick: an example showing syntax for both ports would be great.
This can be included here, leaving the previous example intact. Otherwise, LGTM.
> +               gpio-controller; /* Port 0 as GPIOs */
> +               #gpio-cells = <2>;
> +       };
>
> base-commit: 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff
> --
> 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

Re: [RFC PATCH v2 1/2] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

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

> Hello Hugo,
> 
> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > Some variants in this series of uart controllers have GPIO pins that
> > are shared between GPIO and modem control lines.
> >
> > The pin mux mode (GPIO or modem control lines) can be set for each
> > ports (channels) supported by the variant.
> >
> > This adds a property to the device tree to set the GPIO pin mux to
> > modem control lines on selected ports if needed.
> >
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > v1 -> v2: Add nxp vendor prefix
> >           Add git base-commit
> >
> >  .../bindings/serial/nxp,sc16is7xx.txt         | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > index 0fa8e3e43bf8..23a81c83f012 100644
> > --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > @@ -23,6 +23,9 @@ Optional properties:
> >      1 = active low.
> >  - irda-mode-ports: An array that lists the indices of the port that
> >                    should operate in IrDA mode.
> > +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> > +                               should have shared GPIO lines configured as modem
> > +                               control lines.
> >
> >  Example:
> >          sc16is750: sc16is750@51 {
> > @@ -35,6 +38,17 @@ Example:
> >                  #gpio-cells = <2>;
> >          };
> >
> > +       sc16is752: sc16is752@54 {
> > +               compatible = "nxp,sc16is752";
> > +               reg = <0x54>;
> > +               clocks = <&clk20m>;
> > +               interrupt-parent = <&gpio3>;
> > +               interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> > +               nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> > +               gpio-controller; /* Port 0 as GPIOs */
> > +               #gpio-cells = <2>;
> > +       };
> > +
> >  * spi as bus
> >
> >  Required properties:
> > @@ -59,6 +73,9 @@ Optional properties:
> >      1 = active low.
> >  - irda-mode-ports: An array that lists the indices of the port that
> >                    should operate in IrDA mode.
> > +- nxp,modem-control-line-ports: An array that lists the indices of the port that
> > +                               should have shared GPIO lines configured as modem
> > +                               control lines.
> >
> >  Example:
> >         sc16is750: sc16is750@0 {
> > @@ -70,3 +87,14 @@ Example:
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> >         };
> > +
> > +       sc16is752: sc16is752@0 {
> > +               compatible = "nxp,sc16is752";
> > +               reg = <0>;
> > +               clocks = <&clk20m>;
> > +               interrupt-parent = <&gpio3>;
> > +               interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
> > +               nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
> Just a minor nitpick: an example showing syntax for both ports would be great.
> This can be included here, leaving the previous example intact. Otherwise, LGTM.
> > +               gpio-controller; /* Port 0 as GPIOs */
> > +               #gpio-cells = <2>;

Hi,
I have decided to add a separate example, because adding an example for both ports also means that the gpio-controller property should be removed.

Hugo.


> > +       };
> >
> > base-commit: 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff
> > --
> > 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
> 
>