[PATCH 0/8] serial: st-asc: Allow handling of RTS line

[PATCH 0/8] serial: st-asc: Allow handling of RTS line

[Lee Jones]

When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do.  Thus, we have to do this manually.

This set ensures the correct Pinctrl groups are configured and
obtained for both manual toggling of the RTS line and for the IP to
take over the lines when HW flow-control is requested by the user.

Lee Jones (8):
  serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
  serial: st-asc: Provide RTS functionality
  serial: st-asc: Read in all Pinctrl states
  serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
  ARM: dts: STiH410-b2260: Identify the UART RTS line
  ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
  ARM: dts: STiH407-family: Use new Pinctrl groups
  ARM: dts: STiH407-family: Enable HW flow-control

 arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
 arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
 arch/arm/boot/dts/stih410-b2260.dts    |  1 +
 drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
 4 files changed, 105 insertions(+), 13 deletions(-)

-- 
2.10.2

[PATCH 1/8] serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled

[Lee Jones]

The datasheet states:

"If the MODE field selects an 8-bit frame then this [parity error] bit
 is undefined. Software should ignore this bit when reading 8-bit frames."

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/tty/serial/st-asc.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 379e5bd..69e6232 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -287,9 +287,19 @@ static void asc_transmit_chars(struct uart_port *port)
 static void asc_receive_chars(struct uart_port *port)
 {
 	struct tty_port *tport = &port->state->port;
-	unsigned long status;
+	unsigned long status, mode;
 	unsigned long c = 0;
 	char flag;
+	bool ignore_pe = false;
+
+	/*
+	 * Datasheet states: If the MODE field selects an 8-bit frame then
+	 * this [parity error] bit is undefined. Software should ignore this
+	 * bit when reading 8-bit frames.
+	 */
+	mode = asc_in(port, ASC_CTL) & ASC_CTL_MODE_MSK;
+	if (mode == ASC_CTL_MODE_8BIT || mode == ASC_CTL_MODE_8BIT_PAR)
+		ignore_pe = true;
 
 	if (port->irq_wake)
 		pm_wakeup_event(tport->tty->dev, 0);
@@ -299,8 +309,8 @@ static void asc_receive_chars(struct uart_port *port)
 		flag = TTY_NORMAL;
 		port->icount.rx++;
 
-		if ((c & (ASC_RXBUF_FE | ASC_RXBUF_PE)) ||
-			status & ASC_STA_OE) {
+		if (status & ASC_STA_OE || c & ASC_RXBUF_FE ||
+		    (c & ASC_RXBUF_PE && !ignore_pe)) {
 
 			if (c & ASC_RXBUF_FE) {
 				if (c == (ASC_RXBUF_FE | ASC_RXBUF_DUMMY_RX)) {
-- 
2.10.2

[PATCH 2/8] serial: st-asc: Provide RTS functionality

[Lee Jones]

Until this point, it has not been possible for userland serial
applications (e.g. stty) to toggle the UART RTS line.  This can
be useful with certain configurations. For example, when using
a Mezzanine on a Linaro 96board, RTS line is used to take the
on-board microcontroller in and out of reset.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/tty/serial/st-asc.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 69e6232..397df50 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -30,6 +30,7 @@
 #include <linux/of_platform.h>
 #include <linux/serial_core.h>
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 
 #define DRIVER_NAME "st-asc"
 #define ASC_SERIAL_NAME "ttyAS"
@@ -38,6 +39,7 @@
 
 struct asc_port {
 	struct uart_port port;
+	struct gpio_desc *rts;
 	struct clk *clk;
 	unsigned int hw_flow_control:1;
 	unsigned int force_m1:1;
@@ -391,12 +393,27 @@ static unsigned int asc_tx_empty(struct uart_port *port)
 
 static void asc_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
+	struct asc_port *ascport = to_asc_port(port);
+
 	/*
-	 * This routine is used for seting signals of: DTR, DCD, CTS/RTS
-	 * We use ASC's hardware for CTS/RTS, so don't need any for that.
-	 * Some boards have DTR and DCD implemented using PIO pins,
-	 * code to do this should be hooked in here.
+	 * This routine is used for seting signals of: DTR, DCD, CTS and RTS.
+	 * We use ASC's hardware for CTS/RTS when hardware flow-control is
+	 * enabled, however if the RTS line is required for another purpose,
+	 * commonly controlled using HUP from userspace, then we need to toggle
+	 * it manually, using GPIO.
+	 *
+	 * Some boards also have DTR and DCD implemented using PIO pins, code to
+	 * do this should be hooked in here.
 	 */
+
+	if (!ascport->rts)
+		return;
+
+	/* If HW flow-control is enabled, we can't fiddle with the RTS line */
+	if (asc_in(port, ASC_CTL) & ASC_CTL_CTSENABLE)
+		return;
+
+	gpiod_set_value(ascport->rts, mctrl & TIOCM_RTS);
 }
 
 static unsigned int asc_get_mctrl(struct uart_port *port)
@@ -726,6 +743,8 @@ static struct asc_port *asc_of_get_asc_port(struct platform_device *pdev)
 							"st,hw-flow-control");
 	asc_ports[id].force_m1 =  of_property_read_bool(np, "st,force_m1");
 	asc_ports[id].port.line = id;
+	asc_ports[id].rts = NULL;
+
 	return &asc_ports[id];
 }
 
-- 
2.10.2

[PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

There are now 2 possible separate/different Pinctrl states which can
be provided from platform data.  One which encompasses the lines
required for HW flow-control (CTS/RTS) and another which does not
specify these lines, such that they can be used via GPIO mechanisms
for manually toggling (i.e. from a request by `stty`).

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 397df50..03801ed 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -37,10 +37,16 @@
 #define ASC_FIFO_SIZE 16
 #define ASC_MAX_PORTS 8
 
+/* Pinctrl states */
+#define DEFAULT		0
+#define MANUAL_RTS	1
+
 struct asc_port {
 	struct uart_port port;
 	struct gpio_desc *rts;
 	struct clk *clk;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *states[2];
 	unsigned int hw_flow_control:1;
 	unsigned int force_m1:1;
 };
@@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
 {
 	struct uart_port *port = &ascport->port;
 	struct resource *res;
+	int ret;
 
 	port->iotype	= UPIO_MEM;
 	port->flags	= UPF_BOOT_AUTOCONF;
@@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
 	WARN_ON(ascport->port.uartclk == 0);
 	clk_disable_unprepare(ascport->clk);
 
+	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(ascport->pinctrl)) {
+		ret = PTR_ERR(ascport->pinctrl);
+		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
+	}
+
+	ascport->states[DEFAULT] =
+		pinctrl_lookup_state(ascport->pinctrl, "default");
+	if (IS_ERR(ascport->states[DEFAULT])) {
+		ret = PTR_ERR(ascport->states[DEFAULT]);
+		dev_err(&pdev->dev,
+			"Failed to look up Pinctrl state 'default': %d\n", ret);
+		return ret;
+	}
+
+	/* "manual-rts" state is optional */
+	ascport->states[MANUAL_RTS] =
+		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
+	if (IS_ERR(ascport->states[MANUAL_RTS]))
+		ascport->states[MANUAL_RTS] = NULL;
+
 	return 0;
 }
 
-- 
2.10.2

[PATCH 4/8] serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles

[Lee Jones]

When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do.  Thus, we have to do this manually.

This patch ensures that when HW flow-control is disabled the RTS/CTS
lines are free to be registered via the GPIO API.  It also ensures
any registered GPIO lines are unregistered when HW flow-control is
requested, allowing the IP to control them automatically.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/tty/serial/st-asc.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 03801ed..2c4b5f5 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -512,6 +512,8 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
 			    struct ktermios *old)
 {
 	struct asc_port *ascport = to_asc_port(port);
+	struct device_node *np = port->dev->of_node;
+	struct gpio_desc *gpiod;
 	unsigned int baud;
 	u32 ctrl_val;
 	tcflag_t cflag;
@@ -555,9 +557,32 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
 		ctrl_val |= ASC_CTL_PARITYODD;
 
 	/* hardware flow control */
-	if ((cflag & CRTSCTS))
+	if ((cflag & CRTSCTS)) {
 		ctrl_val |= ASC_CTL_CTSENABLE;
 
+		/* If flow-control selected, stop handling RTS manually */
+		if (ascport->rts) {
+			devm_gpiod_put(port->dev, ascport->rts);
+			ascport->rts = NULL;
+
+			pinctrl_select_state(ascport->pinctrl,
+					     ascport->states[DEFAULT]);
+		}
+	} else {
+		/* If flow-control disabled, it's safe to handle RTS manually */
+		if (!ascport->rts && ascport->states[MANUAL_RTS]) {
+			pinctrl_select_state(ascport->pinctrl,
+					     ascport->states[MANUAL_RTS]);
+
+			gpiod =	devm_get_gpiod_from_child(port->dev, "rts",
+							  &np->fwnode);
+			if (!IS_ERR(gpiod)) {
+				gpiod_direction_output(gpiod, 0);
+				ascport->rts = gpiod;
+			}
+		}
+	}
+
 	if ((baud < 19200) && !ascport->force_m1) {
 		asc_out(port, ASC_BAUDRATE, (port->uartclk / (16 * baud)));
 	} else {
-- 
2.10.2

[PATCH 5/8] ARM: dts: STiH410-b2260: Identify the UART RTS line

[Lee Jones]

When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do.  Thus, we have to do this manually.
This patch configures the UART RTS line as a GPIO for manipulation
within the UART driver when HW flow-control is not enabled.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih410-b2260.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stih410-b2260.dts b/arch/arm/boot/dts/stih410-b2260.dts
index 06b0696..fa16aba 100644
--- a/arch/arm/boot/dts/stih410-b2260.dts
+++ b/arch/arm/boot/dts/stih410-b2260.dts
@@ -63,6 +63,7 @@
 		uart0: serial@9830000 {
 			label = "LS-UART0";
 			status = "okay";
+			rts-gpios = <&pio17 3 GPIO_ACTIVE_LOW>;
 		};
 
 		/* Low speed expansion connector */
-- 
2.10.2

[PATCH 6/8] ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control

[Lee Jones]

Each serial port which supports HW flow-control should have 2 Pinctrl
groups.  Once for when HW flow-control is in progress, where the IP
will take over controlling the lines and another group which enables
the lines to be toggled using GPIO mechanisms.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
index daab16b..fbb7f86 100644
--- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
@@ -465,8 +465,16 @@
 			serial0 {
 				pinctrl_serial0: serial0-0 {
 					st,pins {
-						tx = <&pio17 0 ALT1 OUT>;
-						rx = <&pio17 1 ALT1 IN>;
+						tx =  <&pio17 0 ALT1 OUT>;
+						rx =  <&pio17 1 ALT1 IN>;
+					};
+				};
+				pinctrl_serial0_flowctrl: serial0-0_flowctrl {
+					st,pins {
+						tx =  <&pio17 0 ALT1 OUT>;
+						rx =  <&pio17 1 ALT1 IN>;
+						cts = <&pio17 2 ALT1 IN>;
+						rts = <&pio17 3 ALT1 OUT>;
 					};
 				};
 			};
-- 
2.10.2

[PATCH 7/8] ARM: dts: STiH407-family: Use new Pinctrl groups

[Lee Jones]

Having just defined some new Pinctrl groups for when when HW flow-
control is {en,dis}abled, let's reference them for use within the
driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index c8b2944..9789978 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -222,8 +222,9 @@
 			compatible = "st,asc";
 			reg = <0x9830000 0x2c>;
 			interrupts = <GIC_SPI 122 IRQ_TYPE_NONE>;
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_serial0>;
+			pinctrl-names = "default", "manual-rts";
+			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
+			pinctrl-1 = <&pinctrl_serial0>;
 			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
 
 			status = "disabled";
-- 
2.10.2

[PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

[Lee Jones]

Hardware flow-control capability must be specified at a platform
level in order to inform the ASC driver that the platform is capable
(i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
capable, so let's provide the property.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 9789978..7ada8ea 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -226,7 +226,7 @@
 			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
 			pinctrl-1 = <&pinctrl_serial0>;
 			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
-
+			st,hw-flow-control;
 			status = "disabled";
 		};
 
-- 
2.10.2

Re: [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[kbuild test robot]

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

Hi Lee,

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.10-rc5 next-20170124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lee-Jones/serial-st-asc-Allow-handling-of-RTS-line/20170125-014618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

   drivers/tty/serial/st-asc.c: In function 'asc_init_port':
>> drivers/tty/serial/st-asc.c:730:21: error: implicit declaration of function 'devm_pinctrl_get' [-Werror=implicit-function-declaration]
     ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
                        ^~~~~~~~~~~~~~~~
>> drivers/tty/serial/st-asc.c:730:19: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
                      ^
>> drivers/tty/serial/st-asc.c:737:3: error: implicit declaration of function 'pinctrl_lookup_state' [-Werror=implicit-function-declaration]
      pinctrl_lookup_state(ascport->pinctrl, "default");
      ^~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/st-asc.c:736:27: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->states[DEFAULT] =
                              ^
   drivers/tty/serial/st-asc.c:746:30: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->states[MANUAL_RTS] =
                                 ^
   cc1: some warnings being treated as errors

vim +/devm_pinctrl_get +730 drivers/tty/serial/st-asc.c

   724		/* ensure that clk rate is correct by enabling the clk */
   725		clk_prepare_enable(ascport->clk);
   726		ascport->port.uartclk = clk_get_rate(ascport->clk);
   727		WARN_ON(ascport->port.uartclk == 0);
   728		clk_disable_unprepare(ascport->clk);
   729	
 > 730		ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
   731		if (IS_ERR(ascport->pinctrl)) {
   732			ret = PTR_ERR(ascport->pinctrl);
   733			dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
   734		}
   735	
   736		ascport->states[DEFAULT] =
 > 737			pinctrl_lookup_state(ascport->pinctrl, "default");
   738		if (IS_ERR(ascport->states[DEFAULT])) {
   739			ret = PTR_ERR(ascport->states[DEFAULT]);
   740			dev_err(&pdev->dev,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48646 bytes --]

Re: [PATCH 4/8] serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles

[kbuild test robot]

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

Hi Lee,

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.10-rc5 next-20170124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lee-Jones/serial-st-asc-Allow-handling-of-RTS-line/20170125-014618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   drivers/tty/serial/st-asc.c: In function 'asc_set_termios':
>> drivers/tty/serial/st-asc.c:568:4: error: implicit declaration of function 'pinctrl_select_state' [-Werror=implicit-function-declaration]
       pinctrl_select_state(ascport->pinctrl,
       ^~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/st-asc.c: In function 'asc_init_port':
   drivers/tty/serial/st-asc.c:755:21: error: implicit declaration of function 'devm_pinctrl_get' [-Werror=implicit-function-declaration]
     ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
                        ^~~~~~~~~~~~~~~~
   drivers/tty/serial/st-asc.c:755:19: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
                      ^
   drivers/tty/serial/st-asc.c:762:3: error: implicit declaration of function 'pinctrl_lookup_state' [-Werror=implicit-function-declaration]
      pinctrl_lookup_state(ascport->pinctrl, "default");
      ^~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/st-asc.c:761:27: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->states[DEFAULT] =
                              ^
   drivers/tty/serial/st-asc.c:771:30: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     ascport->states[MANUAL_RTS] =
                                 ^
   cc1: some warnings being treated as errors

vim +/pinctrl_select_state +568 drivers/tty/serial/st-asc.c

   562	
   563			/* If flow-control selected, stop handling RTS manually */
   564			if (ascport->rts) {
   565				devm_gpiod_put(port->dev, ascport->rts);
   566				ascport->rts = NULL;
   567	
 > 568				pinctrl_select_state(ascport->pinctrl,
   569						     ascport->states[DEFAULT]);
   570			}
   571		} else {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48646 bytes --]

Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line

[Greg KH]

On Tue, Jan 24, 2017 at 01:43:02PM +0000, Lee Jones wrote:
> When hardware flow-control is disabled, manual toggling of the UART's
> reset line (RTS) using userland applications (e.g. stty) is not
> possible, since the ASC IP does not provide this functionality in the
> same was as some other IPs do.  Thus, we have to do this manually.
> 
> This set ensures the correct Pinctrl groups are configured and
> obtained for both manual toggling of the RTS line and for the IP to
> take over the lines when HW flow-control is requested by the user.
> 
> Lee Jones (8):
>   serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
>   serial: st-asc: Provide RTS functionality
>   serial: st-asc: Read in all Pinctrl states
>   serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
>   ARM: dts: STiH410-b2260: Identify the UART RTS line
>   ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
>   ARM: dts: STiH407-family: Use new Pinctrl groups
>   ARM: dts: STiH407-family: Enable HW flow-control
> 
>  arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
>  arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
>  arch/arm/boot/dts/stih410-b2260.dts    |  1 +
>  drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
>  4 files changed, 105 insertions(+), 13 deletions(-)
> 
> -- 
> 2.10.2

I'll ignore this series due to the kbuild problems :(

Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

[Peter Griffin]

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> Hardware flow-control capability must be specified at a platform
> level in order to inform the ASC driver that the platform is capable
> (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> capable, so let's provide the property.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index 9789978..7ada8ea 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -226,7 +226,7 @@
>  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
>  			pinctrl-1 = <&pinctrl_serial0>;
>  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> -
> +			st,hw-flow-control;

There is a generic serial binding for this already. As this ST property
hasn't been used upstream, it seems like it would be worth dropping it
and switching to the generic uart-has-rtscts one.

See Documentation/devicetree/bindings/serial/serial.txt

  - uart-has-rtscts: The presence of this property indicates that the
    UART has dedicated lines for RTS/CTS hardware flow control, and that
    they are available for use (wired and enabled by pinmux configuration).
    This depends on both the UART hardware and the board wiring.
    Note that this property is mutually-exclusive with "cts-gpios" and
    "rts-gpios" above.

Also you should put this in the board dtsi, as it is board dependent property.
By putting it here you are enabling hw-flow-control for all stih407-family
based boards.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 6/8] ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control

[Peter Griffin]

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> Each serial port which supports HW flow-control should have 2 Pinctrl
> groups.  Once for when HW flow-control is in progress, where the IP

Nit: Once?

> will take over controlling the lines and another group which enables
> the lines to be toggled using GPIO mechanisms.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Peter Griffin <peter.griffin@linaro.org>

> ---
>  arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> index daab16b..fbb7f86 100644
> --- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> @@ -465,8 +465,16 @@
>  			serial0 {
>  				pinctrl_serial0: serial0-0 {
>  					st,pins {
> -						tx = <&pio17 0 ALT1 OUT>;
> -						rx = <&pio17 1 ALT1 IN>;
> +						tx =  <&pio17 0 ALT1 OUT>;
> +						rx =  <&pio17 1 ALT1 IN>;
> +					};
> +				};
> +				pinctrl_serial0_flowctrl: serial0-0_flowctrl {
> +					st,pins {
> +						tx =  <&pio17 0 ALT1 OUT>;
> +						rx =  <&pio17 1 ALT1 IN>;
> +						cts = <&pio17 2 ALT1 IN>;
> +						rts = <&pio17 3 ALT1 OUT>;
>  					};
>  				};
>  			};

Re: [STLinux Kernel] [PATCH 1/8] serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled

[Peter Griffin]

On Tue, 24 Jan 2017, Lee Jones wrote:

> The datasheet states:
> 
> "If the MODE field selects an 8-bit frame then this [parity error] bit
>  is undefined. Software should ignore this bit when reading 8-bit frames."
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Peter Griffin <peter.griffin@linaro.org>

> ---
>  drivers/tty/serial/st-asc.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 379e5bd..69e6232 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -287,9 +287,19 @@ static void asc_transmit_chars(struct uart_port *port)
>  static void asc_receive_chars(struct uart_port *port)
>  {
>  	struct tty_port *tport = &port->state->port;
> -	unsigned long status;
> +	unsigned long status, mode;
>  	unsigned long c = 0;
>  	char flag;
> +	bool ignore_pe = false;
> +
> +	/*
> +	 * Datasheet states: If the MODE field selects an 8-bit frame then
> +	 * this [parity error] bit is undefined. Software should ignore this
> +	 * bit when reading 8-bit frames.
> +	 */
> +	mode = asc_in(port, ASC_CTL) & ASC_CTL_MODE_MSK;
> +	if (mode == ASC_CTL_MODE_8BIT || mode == ASC_CTL_MODE_8BIT_PAR)
> +		ignore_pe = true;
>  
>  	if (port->irq_wake)
>  		pm_wakeup_event(tport->tty->dev, 0);
> @@ -299,8 +309,8 @@ static void asc_receive_chars(struct uart_port *port)
>  		flag = TTY_NORMAL;
>  		port->icount.rx++;
>  
> -		if ((c & (ASC_RXBUF_FE | ASC_RXBUF_PE)) ||
> -			status & ASC_STA_OE) {
> +		if (status & ASC_STA_OE || c & ASC_RXBUF_FE ||
> +		    (c & ASC_RXBUF_PE && !ignore_pe)) {
>  
>  			if (c & ASC_RXBUF_FE) {
>  				if (c == (ASC_RXBUF_FE | ASC_RXBUF_DUMMY_RX)) {

Re: [STLinux Kernel] [PATCH 2/8] serial: st-asc: Provide RTS functionality

[Peter Griffin]

On Tue, 24 Jan 2017, Lee Jones wrote:

> Until this point, it has not been possible for userland serial
> applications (e.g. stty) to toggle the UART RTS line.  This can
> be useful with certain configurations. For example, when using
> a Mezzanine on a Linaro 96board, RTS line is used to take the
> on-board microcontroller in and out of reset.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Peter Griffin <peter.griffin@linaro.org>

> ---
>  drivers/tty/serial/st-asc.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 69e6232..397df50 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -30,6 +30,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/serial_core.h>
>  #include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
>  
>  #define DRIVER_NAME "st-asc"
>  #define ASC_SERIAL_NAME "ttyAS"
> @@ -38,6 +39,7 @@
>  
>  struct asc_port {
>  	struct uart_port port;
> +	struct gpio_desc *rts;
>  	struct clk *clk;
>  	unsigned int hw_flow_control:1;
>  	unsigned int force_m1:1;
> @@ -391,12 +393,27 @@ static unsigned int asc_tx_empty(struct uart_port *port)
>  
>  static void asc_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
> +	struct asc_port *ascport = to_asc_port(port);
> +
>  	/*
> -	 * This routine is used for seting signals of: DTR, DCD, CTS/RTS
> -	 * We use ASC's hardware for CTS/RTS, so don't need any for that.
> -	 * Some boards have DTR and DCD implemented using PIO pins,
> -	 * code to do this should be hooked in here.
> +	 * This routine is used for seting signals of: DTR, DCD, CTS and RTS.
> +	 * We use ASC's hardware for CTS/RTS when hardware flow-control is
> +	 * enabled, however if the RTS line is required for another purpose,
> +	 * commonly controlled using HUP from userspace, then we need to toggle
> +	 * it manually, using GPIO.
> +	 *
> +	 * Some boards also have DTR and DCD implemented using PIO pins, code to
> +	 * do this should be hooked in here.
>  	 */
> +
> +	if (!ascport->rts)
> +		return;
> +
> +	/* If HW flow-control is enabled, we can't fiddle with the RTS line */
> +	if (asc_in(port, ASC_CTL) & ASC_CTL_CTSENABLE)
> +		return;
> +
> +	gpiod_set_value(ascport->rts, mctrl & TIOCM_RTS);
>  }
>  
>  static unsigned int asc_get_mctrl(struct uart_port *port)
> @@ -726,6 +743,8 @@ static struct asc_port *asc_of_get_asc_port(struct platform_device *pdev)
>  							"st,hw-flow-control");
>  	asc_ports[id].force_m1 =  of_property_read_bool(np, "st,force_m1");
>  	asc_ports[id].port.line = id;
> +	asc_ports[id].rts = NULL;
> +
>  	return &asc_ports[id];
>  }
>  

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> There are now 2 possible separate/different Pinctrl states which can
> be provided from platform data.  One which encompasses the lines
> required for HW flow-control (CTS/RTS) and another which does not
> specify these lines, such that they can be used via GPIO mechanisms
> for manually toggling (i.e. from a request by `stty`).
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 397df50..03801ed 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -37,10 +37,16 @@
>  #define ASC_FIFO_SIZE 16
>  #define ASC_MAX_PORTS 8
>  
> +/* Pinctrl states */
> +#define DEFAULT		0
> +#define MANUAL_RTS	1

Nit: Would be better to have them aligned.

> +
>  struct asc_port {
>  	struct uart_port port;
>  	struct gpio_desc *rts;
>  	struct clk *clk;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *states[2];
>  	unsigned int hw_flow_control:1;
>  	unsigned int force_m1:1;
>  };
> @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
>  {
>  	struct uart_port *port = &ascport->port;
>  	struct resource *res;
> +	int ret;
>  
>  	port->iotype	= UPIO_MEM;
>  	port->flags	= UPF_BOOT_AUTOCONF;
> @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
>  	WARN_ON(ascport->port.uartclk == 0);
>  	clk_disable_unprepare(ascport->clk);
>  
> +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(ascport->pinctrl)) {
> +		ret = PTR_ERR(ascport->pinctrl);
> +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> +	}
> +
> +	ascport->states[DEFAULT] =
> +		pinctrl_lookup_state(ascport->pinctrl, "default");
> +	if (IS_ERR(ascport->states[DEFAULT])) {
> +		ret = PTR_ERR(ascport->states[DEFAULT]);
> +		dev_err(&pdev->dev,
> +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* "manual-rts" state is optional */
> +	ascport->states[MANUAL_RTS] =
> +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> +		ascport->states[MANUAL_RTS] = NULL;
> +

The different pinctrl states looks like a neat solution to the problem.

My only concern here is that 'default' state is implying a hw-flow-control
pinmux config, and manual-rts is implying what is the current upstream
'default' pinmux config.

Which maybe ok if you update all uarts, but currently only serial0
is updated. So the other uarts current 'default' is actually the same as serial0
'manual-rts' grouping, which conceptually is odd.

Would it not be better to make 'manual-rts' the default state? As that aligns
to what is currently already the default for the other UARTS? And then make
hw-flow-control the optional state for serial0?

That also has the advantage that 'default' has the same meaning with older DT's.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 4/8] serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles

[Peter Griffin]

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> When hardware flow-control is disabled, manual toggling of the UART's
> reset line (RTS) using userland applications (e.g. stty) is not
> possible, since the ASC IP does not provide this functionality in the
> same was as some other IPs do.  Thus, we have to do this manually.
> 
> This patch ensures that when HW flow-control is disabled the RTS/CTS
> lines are free to be registered via the GPIO API.  It also ensures
> any registered GPIO lines are unregistered when HW flow-control is
> requested, allowing the IP to control them automatically.

Apart from my comment in previous mail about switching the meaning of
default & manual-rts groupings. It seems like a neat solution.

Acked-by: Peter Griffin <peter.griffin@linaro.org>

> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/tty/serial/st-asc.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 03801ed..2c4b5f5 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -512,6 +512,8 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  			    struct ktermios *old)
>  {
>  	struct asc_port *ascport = to_asc_port(port);
> +	struct device_node *np = port->dev->of_node;
> +	struct gpio_desc *gpiod;
>  	unsigned int baud;
>  	u32 ctrl_val;
>  	tcflag_t cflag;
> @@ -555,9 +557,32 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  		ctrl_val |= ASC_CTL_PARITYODD;
>  
>  	/* hardware flow control */
> -	if ((cflag & CRTSCTS))
> +	if ((cflag & CRTSCTS)) {
>  		ctrl_val |= ASC_CTL_CTSENABLE;
>  
> +		/* If flow-control selected, stop handling RTS manually */
> +		if (ascport->rts) {
> +			devm_gpiod_put(port->dev, ascport->rts);
> +			ascport->rts = NULL;
> +
> +			pinctrl_select_state(ascport->pinctrl,
> +					     ascport->states[DEFAULT]);
> +		}
> +	} else {
> +		/* If flow-control disabled, it's safe to handle RTS manually */
> +		if (!ascport->rts && ascport->states[MANUAL_RTS]) {
> +			pinctrl_select_state(ascport->pinctrl,
> +					     ascport->states[MANUAL_RTS]);
> +
> +			gpiod =	devm_get_gpiod_from_child(port->dev, "rts",
> +							  &np->fwnode);
> +			if (!IS_ERR(gpiod)) {
> +				gpiod_direction_output(gpiod, 0);
> +				ascport->rts = gpiod;
> +			}
> +		}
> +	}
> +
>  	if ((baud < 19200) && !ascport->force_m1) {
>  		asc_out(port, ASC_BAUDRATE, (port->uartclk / (16 * baud)));
>  	} else {

Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

[Peter Griffin]

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > Hardware flow-control capability must be specified at a platform
> > level in order to inform the ASC driver that the platform is capable
> > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > capable, so let's provide the property.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index 9789978..7ada8ea 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -226,7 +226,7 @@
> >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> >  			pinctrl-1 = <&pinctrl_serial0>;
> >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > -
> > +			st,hw-flow-control;
> 
> There is a generic serial binding for this already. As this ST property
> hasn't been used upstream, it seems like it would be worth dropping it
> and switching to the generic uart-has-rtscts one.
> 
> See Documentation/devicetree/bindings/serial/serial.txt
> 
>   - uart-has-rtscts: The presence of this property indicates that the
>     UART has dedicated lines for RTS/CTS hardware flow control, and that
>     they are available for use (wired and enabled by pinmux configuration).
>     This depends on both the UART hardware and the board wiring.
>     Note that this property is mutually-exclusive with "cts-gpios" and
>     "rts-gpios" above.

Thinking some more this generic binding definition is a bit restrictive,
particularly the "Note that this property is mutually-exclusive with "cts-gpios"
and "rts-gpios"" part.

As with this series we will have dynamic on-the-fly changing from hw flow
control to gpio control for other configs which the HW IP doesn't support.

IMO the uart-has-rtscts definition should be updated to reflect that they can
be used together in certain circumstances.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 7/8] ARM: dts: STiH407-family: Use new Pinctrl groups

[Peter Griffin]

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> Having just defined some new Pinctrl groups for when when HW flow-
> control is {en,dis}abled, let's reference them for use within the
> driver.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Same as previous comment, your enabling hw flow control for all
stih407 family boards. I've not checked the schematics for them
but as hw flow control is dependent on board wiring, IMO this
should be added in board specific file.

regards,

Peter.

> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index c8b2944..9789978 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -222,8 +222,9 @@
>  			compatible = "st,asc";
>  			reg = <0x9830000 0x2c>;
>  			interrupts = <GIC_SPI 122 IRQ_TYPE_NONE>;
> -			pinctrl-names = "default";
> -			pinctrl-0 = <&pinctrl_serial0>;
> +			pinctrl-names = "default", "manual-rts";
> +			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> +			pinctrl-1 = <&pinctrl_serial0>;
>  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
>  
>  			status = "disabled";

Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line

[Lee Jones]

On Wed, 25 Jan 2017, Greg KH wrote:

> On Tue, Jan 24, 2017 at 01:43:02PM +0000, Lee Jones wrote:
> > When hardware flow-control is disabled, manual toggling of the UART's
> > reset line (RTS) using userland applications (e.g. stty) is not
> > possible, since the ASC IP does not provide this functionality in the
> > same was as some other IPs do.  Thus, we have to do this manually.
> > 
> > This set ensures the correct Pinctrl groups are configured and
> > obtained for both manual toggling of the RTS line and for the IP to
> > take over the lines when HW flow-control is requested by the user.
> > 
> > Lee Jones (8):
> >   serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
> >   serial: st-asc: Provide RTS functionality
> >   serial: st-asc: Read in all Pinctrl states
> >   serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
> >   ARM: dts: STiH410-b2260: Identify the UART RTS line
> >   ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
> >   ARM: dts: STiH407-family: Use new Pinctrl groups
> >   ARM: dts: STiH407-family: Enable HW flow-control
> > 
> >  arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
> >  arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
> >  arch/arm/boot/dts/stih410-b2260.dts    |  1 +
> >  drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
> >  4 files changed, 105 insertions(+), 13 deletions(-)
> > 
> 
> I'll ignore this series due to the kbuild problems :(

It's strange.

These didn't happen locally ... and aiaiai was also happy.

Nevertheless, I will fix-up and resend.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 7/8] ARM: dts: STiH407-family: Use new Pinctrl groups

[Lee Jones]

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > Having just defined some new Pinctrl groups for when when HW flow-
> > control is {en,dis}abled, let's reference them for use within the
> > driver.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Same as previous comment, your enabling hw flow control for all
> stih407 family boards. I've not checked the schematics for them
> but as hw flow control is dependent on board wiring, IMO this
> should be added in board specific file.

Fair shout.

On the B2120 UART0 is hooked up to the Smart Card Reader.

Will fix.

> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index c8b2944..9789978 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -222,8 +222,9 @@
> >  			compatible = "st,asc";
> >  			reg = <0x9830000 0x2c>;
> >  			interrupts = <GIC_SPI 122 IRQ_TYPE_NONE>;
> > -			pinctrl-names = "default";
> > -			pinctrl-0 = <&pinctrl_serial0>;
> > +			pinctrl-names = "default", "manual-rts";
> > +			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> > +			pinctrl-1 = <&pinctrl_serial0>;
> >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> >  
> >  			status = "disabled";

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 7/8] ARM: dts: STiH407-family: Use new Pinctrl groups

[Lee Jones]

On Fri, 27 Jan 2017, Lee Jones wrote:

> On Wed, 25 Jan 2017, Peter Griffin wrote:
> 
> > Hi Lee,
> > 
> > On Tue, 24 Jan 2017, Lee Jones wrote:
> > 
> > > Having just defined some new Pinctrl groups for when when HW flow-
> > > control is {en,dis}abled, let's reference them for use within the
> > > driver.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Same as previous comment, your enabling hw flow control for all
> > stih407 family boards. I've not checked the schematics for them
> > but as hw flow control is dependent on board wiring, IMO this
> > should be added in board specific file.
> 
> Fair shout.
> 
> On the B2120 UART0 is hooked up to the Smart Card Reader.
> 
> Will fix.

Actually, the comment above is in regards to the st,hw-flow-control
property [8/8].  The UART0 Pinctrl settings here are correct for all
supported STiH407-family boards.

Will not fix.

> > > ---
> > >  arch/arm/boot/dts/stih407-family.dtsi | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > > index c8b2944..9789978 100644
> > > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > > @@ -222,8 +222,9 @@
> > >  			compatible = "st,asc";
> > >  			reg = <0x9830000 0x2c>;
> > >  			interrupts = <GIC_SPI 122 IRQ_TYPE_NONE>;
> > > -			pinctrl-names = "default";
> > > -			pinctrl-0 = <&pinctrl_serial0>;
> > > +			pinctrl-names = "default", "manual-rts";
> > > +			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> > > +			pinctrl-1 = <&pinctrl_serial0>;
> > >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > >  
> > >  			status = "disabled";
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

[Lee Jones]

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > Hardware flow-control capability must be specified at a platform
> > level in order to inform the ASC driver that the platform is capable
> > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > capable, so let's provide the property.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index 9789978..7ada8ea 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -226,7 +226,7 @@
> >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> >  			pinctrl-1 = <&pinctrl_serial0>;
> >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > -
> > +			st,hw-flow-control;
> 
> There is a generic serial binding for this already. As this ST property
> hasn't been used upstream, it seems like it would be worth dropping it
> and switching to the generic uart-has-rtscts one.
> 
> See Documentation/devicetree/bindings/serial/serial.txt
> 
>   - uart-has-rtscts: The presence of this property indicates that the
>     UART has dedicated lines for RTS/CTS hardware flow control, and that
>     they are available for use (wired and enabled by pinmux configuration).
>     This depends on both the UART hardware and the board wiring.
>     Note that this property is mutually-exclusive with "cts-gpios" and
>     "rts-gpios" above.

I've done some digging and I can't see anywhere where
st,hw-flow-control is being used, even in the BSP kernel(s).  It's
also not documented in dt-bindings.  With that in mind, I think it's
probably okay to use the generic binding.

Although, the "Note" at the bottom of the uart-has-rtscts is not
correct in our case, since we handle the case dynamically.

> Also you should put this in the board dtsi, as it is board dependent property.
> By putting it here you are enabling hw-flow-control for all stih407-family
> based boards.

Fair shout.
  
On the B2120 UART0 is hooked up to the Smart Card Reader.
  
Will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

[Lee Jones]

On Wed, 25 Jan 2017, Peter Griffin wrote:
> On Wed, 25 Jan 2017, Peter Griffin wrote:
> > On Tue, 24 Jan 2017, Lee Jones wrote:
> > 
> > > Hardware flow-control capability must be specified at a platform
> > > level in order to inform the ASC driver that the platform is capable
> > > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > > capable, so let's provide the property.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > > index 9789978..7ada8ea 100644
> > > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > > @@ -226,7 +226,7 @@
> > >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> > >  			pinctrl-1 = <&pinctrl_serial0>;
> > >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > > -
> > > +			st,hw-flow-control;
> > 
> > There is a generic serial binding for this already. As this ST property
> > hasn't been used upstream, it seems like it would be worth dropping it
> > and switching to the generic uart-has-rtscts one.
> > 
> > See Documentation/devicetree/bindings/serial/serial.txt
> > 
> >   - uart-has-rtscts: The presence of this property indicates that the
> >     UART has dedicated lines for RTS/CTS hardware flow control, and that
> >     they are available for use (wired and enabled by pinmux configuration).
> >     This depends on both the UART hardware and the board wiring.
> >     Note that this property is mutually-exclusive with "cts-gpios" and
> >     "rts-gpios" above.
> 
> Thinking some more this generic binding definition is a bit restrictive,
> particularly the "Note that this property is mutually-exclusive with "cts-gpios"
> and "rts-gpios"" part.
> 
> As with this series we will have dynamic on-the-fly changing from hw flow
> control to gpio control for other configs which the HW IP doesn't support.
> 
> IMO the uart-has-rtscts definition should be updated to reflect that they can
> be used together in certain circumstances.

Ah, you noticed that too.  Yes, will update the doc.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > There are now 2 possible separate/different Pinctrl states which can
> > be provided from platform data.  One which encompasses the lines
> > required for HW flow-control (CTS/RTS) and another which does not
> > specify these lines, such that they can be used via GPIO mechanisms
> > for manually toggling (i.e. from a request by `stty`).
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > index 397df50..03801ed 100644
> > --- a/drivers/tty/serial/st-asc.c
> > +++ b/drivers/tty/serial/st-asc.c
> > @@ -37,10 +37,16 @@
> >  #define ASC_FIFO_SIZE 16
> >  #define ASC_MAX_PORTS 8
> >  
> > +/* Pinctrl states */
> > +#define DEFAULT		0
> > +#define MANUAL_RTS	1
> 
> Nit: Would be better to have them aligned.

These are aligned in code.  The patch format throws things out.

Look, same lines with the "> > +" removed:

/* Pinctrl states */
#define DEFAULT		0
#define MANUAL_RTS	1

Try it.

> > +
> >  struct asc_port {
> >  	struct uart_port port;
> >  	struct gpio_desc *rts;
> >  	struct clk *clk;
> > +	struct pinctrl *pinctrl;
> > +	struct pinctrl_state *states[2];
> >  	unsigned int hw_flow_control:1;
> >  	unsigned int force_m1:1;
> >  };
> > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> >  {
> >  	struct uart_port *port = &ascport->port;
> >  	struct resource *res;
> > +	int ret;
> >  
> >  	port->iotype	= UPIO_MEM;
> >  	port->flags	= UPF_BOOT_AUTOCONF;
> > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> >  	WARN_ON(ascport->port.uartclk == 0);
> >  	clk_disable_unprepare(ascport->clk);
> >  
> > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (IS_ERR(ascport->pinctrl)) {
> > +		ret = PTR_ERR(ascport->pinctrl);
> > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > +	}
> > +
> > +	ascport->states[DEFAULT] =
> > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > +		dev_err(&pdev->dev,
> > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* "manual-rts" state is optional */
> > +	ascport->states[MANUAL_RTS] =
> > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > +		ascport->states[MANUAL_RTS] = NULL;
> > +
> 
> The different pinctrl states looks like a neat solution to the problem.
> 
> My only concern here is that 'default' state is implying a hw-flow-control
> pinmux config, and manual-rts is implying what is the current upstream
> 'default' pinmux config.
> 
> Which maybe ok if you update all uarts, but currently only serial0
> is updated. So the other uarts current 'default' is actually the same as serial0
> 'manual-rts' grouping, which conceptually is odd.
> 
> Would it not be better to make 'manual-rts' the default state? As that aligns
> to what is currently already the default for the other UARTS? And then make
> hw-flow-control the optional state for serial0?
> 
> That also has the advantage that 'default' has the same meaning with older DT's.

The reason it was done is this was because none of the other UARTs
require 2 separate Pinctrl configurations, only this one.  Moreover,
if they support RTS/CTS then I believe that the lines should be
defined in Pinctrl.  Thus, it was my plan to update all UART's default
Pinctrl configs to include the RTS/CTS lines.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Fri, 27 Jan 2017, Lee Jones wrote:

> On Wed, 25 Jan 2017, Peter Griffin wrote:
> 
> > Hi Lee,
> > 
> > On Tue, 24 Jan 2017, Lee Jones wrote:
> > 
> > > There are now 2 possible separate/different Pinctrl states which can
> > > be provided from platform data.  One which encompasses the lines
> > > required for HW flow-control (CTS/RTS) and another which does not
> > > specify these lines, such that they can be used via GPIO mechanisms
> > > for manually toggling (i.e. from a request by `stty`).
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > index 397df50..03801ed 100644
> > > --- a/drivers/tty/serial/st-asc.c
> > > +++ b/drivers/tty/serial/st-asc.c
> > > @@ -37,10 +37,16 @@
> > >  #define ASC_FIFO_SIZE 16
> > >  #define ASC_MAX_PORTS 8
> > >  
> > > +/* Pinctrl states */
> > > +#define DEFAULT		0
> > > +#define MANUAL_RTS	1
> > 
> > Nit: Would be better to have them aligned.
> 
> These are aligned in code.  The patch format throws things out.
> 
> Look, same lines with the "> > +" removed:

Ah OK.

> 
> /* Pinctrl states */
> #define DEFAULT		0
> #define MANUAL_RTS	1
> 
> Try it.
> 
> > > +
> > >  struct asc_port {
> > >  	struct uart_port port;
> > >  	struct gpio_desc *rts;
> > >  	struct clk *clk;
> > > +	struct pinctrl *pinctrl;
> > > +	struct pinctrl_state *states[2];
> > >  	unsigned int hw_flow_control:1;
> > >  	unsigned int force_m1:1;
> > >  };
> > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > >  {
> > >  	struct uart_port *port = &ascport->port;
> > >  	struct resource *res;
> > > +	int ret;
> > >  
> > >  	port->iotype	= UPIO_MEM;
> > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > >  	WARN_ON(ascport->port.uartclk == 0);
> > >  	clk_disable_unprepare(ascport->clk);
> > >  
> > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > +	if (IS_ERR(ascport->pinctrl)) {
> > > +		ret = PTR_ERR(ascport->pinctrl);
> > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > +	}
> > > +
> > > +	ascport->states[DEFAULT] =
> > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > +		dev_err(&pdev->dev,
> > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* "manual-rts" state is optional */
> > > +	ascport->states[MANUAL_RTS] =
> > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > +		ascport->states[MANUAL_RTS] = NULL;
> > > +
> > 
> > The different pinctrl states looks like a neat solution to the problem.
> > 
> > My only concern here is that 'default' state is implying a hw-flow-control
> > pinmux config, and manual-rts is implying what is the current upstream
> > 'default' pinmux config.
> > 
> > Which maybe ok if you update all uarts, but currently only serial0
> > is updated. So the other uarts current 'default' is actually the same as serial0
> > 'manual-rts' grouping, which conceptually is odd.
> > 
> > Would it not be better to make 'manual-rts' the default state? As that aligns
> > to what is currently already the default for the other UARTS? And then make
> > hw-flow-control the optional state for serial0?
> > 
> > That also has the advantage that 'default' has the same meaning with older DT's.
> 
> The reason it was done is this was because none of the other UARTs
> require 2 separate Pinctrl configurations, only this one.  Moreover,
> if they support RTS/CTS then I believe that the lines should be
> defined in Pinctrl.

Yes I agree with that.

> Thus, it was my plan to update all UART's default
> Pinctrl configs to include the RTS/CTS lines.
> 

I still don't see the point in changing the meaning of 'default' group and breaking
ABI if you don't need to?

As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
groups you get all the benefits of this series whilst also maintaining backwards
compatbility with older DT's.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

On Mon, 30 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Fri, 27 Jan 2017, Lee Jones wrote:
> 
> > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > 
> > > Hi Lee,
> > > 
> > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > 
> > > > There are now 2 possible separate/different Pinctrl states which can
> > > > be provided from platform data.  One which encompasses the lines
> > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > for manually toggling (i.e. from a request by `stty`).
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > > 
> > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > index 397df50..03801ed 100644
> > > > --- a/drivers/tty/serial/st-asc.c
> > > > +++ b/drivers/tty/serial/st-asc.c
> > > > @@ -37,10 +37,16 @@
> > > >  #define ASC_FIFO_SIZE 16
> > > >  #define ASC_MAX_PORTS 8
> > > >  
> > > > +/* Pinctrl states */
> > > > +#define DEFAULT		0
> > > > +#define MANUAL_RTS	1
> > > 
> > > Nit: Would be better to have them aligned.
> > 
> > These are aligned in code.  The patch format throws things out.
> > 
> > Look, same lines with the "> > +" removed:
> 
> Ah OK.
> 
> > 
> > /* Pinctrl states */
> > #define DEFAULT		0
> > #define MANUAL_RTS	1
> > 
> > Try it.
> > 
> > > > +
> > > >  struct asc_port {
> > > >  	struct uart_port port;
> > > >  	struct gpio_desc *rts;
> > > >  	struct clk *clk;
> > > > +	struct pinctrl *pinctrl;
> > > > +	struct pinctrl_state *states[2];
> > > >  	unsigned int hw_flow_control:1;
> > > >  	unsigned int force_m1:1;
> > > >  };
> > > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > > >  {
> > > >  	struct uart_port *port = &ascport->port;
> > > >  	struct resource *res;
> > > > +	int ret;
> > > >  
> > > >  	port->iotype	= UPIO_MEM;
> > > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > > >  	WARN_ON(ascport->port.uartclk == 0);
> > > >  	clk_disable_unprepare(ascport->clk);
> > > >  
> > > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > > +	if (IS_ERR(ascport->pinctrl)) {
> > > > +		ret = PTR_ERR(ascport->pinctrl);
> > > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > > +	}
> > > > +
> > > > +	ascport->states[DEFAULT] =
> > > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > > +		dev_err(&pdev->dev,
> > > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* "manual-rts" state is optional */
> > > > +	ascport->states[MANUAL_RTS] =
> > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > +
> > > 
> > > The different pinctrl states looks like a neat solution to the problem.
> > > 
> > > My only concern here is that 'default' state is implying a hw-flow-control
> > > pinmux config, and manual-rts is implying what is the current upstream
> > > 'default' pinmux config.
> > > 
> > > Which maybe ok if you update all uarts, but currently only serial0
> > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > 'manual-rts' grouping, which conceptually is odd.
> > > 
> > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > to what is currently already the default for the other UARTS? And then make
> > > hw-flow-control the optional state for serial0?
> > > 
> > > That also has the advantage that 'default' has the same meaning with older DT's.
> > 
> > The reason it was done is this was because none of the other UARTs
> > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > if they support RTS/CTS then I believe that the lines should be
> > defined in Pinctrl.
> 
> Yes I agree with that.
> 
> > Thus, it was my plan to update all UART's default
> > Pinctrl configs to include the RTS/CTS lines.
> > 
> 
> I still don't see the point in changing the meaning of 'default' group and breaking
> ABI if you don't need to?
> 
> As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> groups you get all the benefits of this series whilst also maintaining backwards
> compatbility with older DT's.

What makes you think this will break ABI?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Mon, 30 Jan 2017, Lee Jones wrote:

> On Mon, 30 Jan 2017, Peter Griffin wrote:
> 
> > Hi Lee,
> > 
> > On Fri, 27 Jan 2017, Lee Jones wrote:
> > 
> > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > 
> > > > Hi Lee,
> > > > 
> > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > 
> > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > be provided from platform data.  One which encompasses the lines
> > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > index 397df50..03801ed 100644
> > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > +++ b/drivers/tty/serial/st-asc.c
> > > > > @@ -37,10 +37,16 @@
> > > > >  #define ASC_FIFO_SIZE 16
> > > > >  #define ASC_MAX_PORTS 8
> > > > >  
> > > > > +/* Pinctrl states */
> > > > > +#define DEFAULT		0
> > > > > +#define MANUAL_RTS	1
> > > > 
> > > > Nit: Would be better to have them aligned.
> > > 
> > > These are aligned in code.  The patch format throws things out.
> > > 
> > > Look, same lines with the "> > +" removed:
> > 
> > Ah OK.
> > 
> > > 
> > > /* Pinctrl states */
> > > #define DEFAULT		0
> > > #define MANUAL_RTS	1
> > > 
> > > Try it.
> > > 
> > > > > +
> > > > >  struct asc_port {
> > > > >  	struct uart_port port;
> > > > >  	struct gpio_desc *rts;
> > > > >  	struct clk *clk;
> > > > > +	struct pinctrl *pinctrl;
> > > > > +	struct pinctrl_state *states[2];
> > > > >  	unsigned int hw_flow_control:1;
> > > > >  	unsigned int force_m1:1;
> > > > >  };
> > > > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > > > >  {
> > > > >  	struct uart_port *port = &ascport->port;
> > > > >  	struct resource *res;
> > > > > +	int ret;
> > > > >  
> > > > >  	port->iotype	= UPIO_MEM;
> > > > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > > > >  	WARN_ON(ascport->port.uartclk == 0);
> > > > >  	clk_disable_unprepare(ascport->clk);
> > > > >  
> > > > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > > > +	if (IS_ERR(ascport->pinctrl)) {
> > > > > +		ret = PTR_ERR(ascport->pinctrl);
> > > > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > > > +	}
> > > > > +
> > > > > +	ascport->states[DEFAULT] =
> > > > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > > > +		dev_err(&pdev->dev,
> > > > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	/* "manual-rts" state is optional */
> > > > > +	ascport->states[MANUAL_RTS] =
> > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > +
> > > > 
> > > > The different pinctrl states looks like a neat solution to the problem.
> > > > 
> > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > 'default' pinmux config.
> > > > 
> > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > 'manual-rts' grouping, which conceptually is odd.
> > > > 
> > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > to what is currently already the default for the other UARTS? And then make
> > > > hw-flow-control the optional state for serial0?
> > > > 
> > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > 
> > > The reason it was done is this was because none of the other UARTs
> > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > if they support RTS/CTS then I believe that the lines should be
> > > defined in Pinctrl.
> > 
> > Yes I agree with that.
> > 
> > > Thus, it was my plan to update all UART's default
> > > Pinctrl configs to include the RTS/CTS lines.
> > > 
> > 
> > I still don't see the point in changing the meaning of 'default' group and breaking
> > ABI if you don't need to?
> > 
> > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > groups you get all the benefits of this series whilst also maintaining backwards
> > compatbility with older DT's.
> 
> What makes you think this will break ABI?
>

I've not tried it, but an older DT defines one group, 'default' which contains
the same pin config as your new optional 'manual-rts' group.

The driver now reads like the manual-rts pin config is optional and should be stored in
ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
group and it will be stored in ascport->states[DEFAULT].

That seems wrong to me, and if it executes OK it wouldn't be what you
expect by reading the code.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

On Mon, 30 Jan 2017, Peter Griffin wrote:
> On Mon, 30 Jan 2017, Lee Jones wrote:
> > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > 
> > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > 
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > >  1 file changed, 28 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > index 397df50..03801ed 100644
> > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > +++ b/drivers/tty/serial/st-asc.c

[...]

> > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > +
> > > > > 
> > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > 
> > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > 'default' pinmux config.
> > > > > 
> > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > 
> > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > to what is currently already the default for the other UARTS? And then make
> > > > > hw-flow-control the optional state for serial0?
> > > > > 
> > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > 
> > > > The reason it was done is this was because none of the other UARTs
> > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > if they support RTS/CTS then I believe that the lines should be
> > > > defined in Pinctrl.
> > > 
> > > Yes I agree with that.
> > > 
> > > > Thus, it was my plan to update all UART's default
> > > > Pinctrl configs to include the RTS/CTS lines.
> > > > 
> > > 
> > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > ABI if you don't need to?
> > > 
> > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > groups you get all the benefits of this series whilst also maintaining backwards
> > > compatbility with older DT's.
> > 
> > What makes you think this will break ABI?
> 
> I've not tried it, but an older DT defines one group, 'default' which contains
> the same pin config as your new optional 'manual-rts' group.
> 
> The driver now reads like the manual-rts pin config is optional and should be stored in
> ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> group and it will be stored in ascport->states[DEFAULT].
> 
> That seems wrong to me, and if it executes OK it wouldn't be what you
> expect by reading the code.

This makes no sense at a functional level.

Old kernel, old DTB:

ASC driver doesn't understand Pinctrl, but since only the "default"
state is defined, that's what will be used as a matter of course.
RTS/CTS aren't configured, but that doesn't matter because the DTS
does not advertise that HW flow-control is available.  In this
use-case neither HW flow-control, nor manual toggling of the RTS line
is possible.

New kernel, old DTB:

ASC driver demands "default" and requests "manual-rts" Pinctrl states,
but "manual-rts" isn't available so "default" will be the only
utilised state.  Unlike the first example above, "default" now
contains the RTS and CTS lines, but since the DTS does not advertise
HW flow-control as available they will be harmlessly unused.  This
configuration culminates in the same result as the first example
i.e. no HW flow-control and no manual toggling.  However, there are no
detremental effects to the driver's functions. 

Old kernel, new DTB:

ASC doesn't understand Pinctrl, so the "default" state will be used,
meaning that all of the lines will be configured.  The DTB does
advertise HW flow-control as being available this time, but not in a
way that the old ASC driver understands.  Thus, just as in the two
preceding examples, HW flow-control and manual toggling of the RTS
line is not possible, but again the driver is otherwise fully
functional and doesn't suffer any ill effects from the RTS and CTS
lines being configured.

New kernel, new DTB:

ASC driver demands "default" and requests "manual-rts" Pinctrl
states.  If DTS advertises that HW flow-control is possible and the
client requests it, ASC will use the "default" state and HW
flow-control will commence.  If HW flow-control is not requested by
the client and "manual-rts" is available, then ASC will request the
RTS line is handled by GPIO until such times as the client requests
HW flow-control, at which point ASC will disable GPIO and request the
"default" state again.

It is not possible to read C-code and make assumptions that the DTB
will be in a particular state as you suggest.  No disparity ever
exists and the code is always clear IMHO.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Mon, 30 Jan 2017, Lee Jones wrote:

> On Mon, 30 Jan 2017, Peter Griffin wrote:
> > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > > 
> > > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > > 
> > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > > index 397df50..03801ed 100644
> > > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > > +++ b/drivers/tty/serial/st-asc.c
> 
> [...]
> 
> > > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > > +
> > > > > > 
> > > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > > 
> > > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > > 'default' pinmux config.
> > > > > > 
> > > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > > 
> > > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > > to what is currently already the default for the other UARTS? And then make
> > > > > > hw-flow-control the optional state for serial0?
> > > > > > 
> > > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > > 
> > > > > The reason it was done is this was because none of the other UARTs
> > > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > > if they support RTS/CTS then I believe that the lines should be
> > > > > defined in Pinctrl.
> > > > 
> > > > Yes I agree with that.
> > > > 
> > > > > Thus, it was my plan to update all UART's default
> > > > > Pinctrl configs to include the RTS/CTS lines.
> > > > > 
> > > > 
> > > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > > ABI if you don't need to?
> > > > 
> > > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > > groups you get all the benefits of this series whilst also maintaining backwards
> > > > compatbility with older DT's.
> > > 
> > > What makes you think this will break ABI?
> > 
> > I've not tried it, but an older DT defines one group, 'default' which contains
> > the same pin config as your new optional 'manual-rts' group.
> > 
> > The driver now reads like the manual-rts pin config is optional and should be stored in
> > ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> > group and it will be stored in ascport->states[DEFAULT].
> > 
> > That seems wrong to me, and if it executes OK it wouldn't be what you
> > expect by reading the code.
> 
> This makes no sense at a functional level.
> 
> Old kernel, old DTB:
> 
> ASC driver doesn't understand Pinctrl, but since only the "default"
> state is defined, that's what will be used as a matter of course.
> RTS/CTS aren't configured, but that doesn't matter because the DTS
> does not advertise that HW flow-control is available.  In this
> use-case neither HW flow-control, nor manual toggling of the RTS line
> is possible.
> 
> New kernel, old DTB:
> 
> ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> but "manual-rts" isn't available so "default" will be the only
> utilised state.  Unlike the first example above, "default" now
> contains the RTS and CTS lines,

No it doesn't, default just contains 'tx' & 'rx' pins, as it has always
done until now.

Which is IMO where the condusion arises, as it is the same pin configuration
as what you are now calling 'manual-rts' which the driver just tried and failed
to obtain (although in reality it has actually obtained those pins but stored
them in DEFAULT instead.

I presume this is why it didn't make sense to you above.

>but since the DTS does not advertise
> HW flow-control as available they will be harmlessly unused.  This
> configuration culminates in the same result as the first example
> i.e. no HW flow-control and no manual toggling.  However, there are no
> detremental effects to the driver's functions. 
>

<snip>

>New kernel, new DTB:
> 
> ASC driver demands "default" and requests "manual-rts" Pinctrl
> states.  If DTS advertises that HW flow-control is possible and the
> client requests it, ASC will use the "default" state and HW
> flow-control will commence.  If HW flow-control is not requested by
> the client and "manual-rts" is available, then ASC will request the
> RTS line is handled by GPIO until such times as the client requests
> HW flow-control, at which point ASC will disable GPIO and request the
> "default" state again.

Unless it is uart 1 or 2, in which case 'default' still only contains
tx & rx pins, and you have the same situation as above. 

> 
> It is not possible to read C-code and make assumptions that the DTB
> will be in a particular state as you suggest.
> No disparity ever
> exists and the code is always clear IMHO.
> 

Really?

ascport->states[DEFAULT]: may contain "tx, rx" or "tx, rx, cts & rts"
ascport->states[MANUAL_RTS]: may contain "tx, rx", or it could be stored in DEFAULT

And as the series currently is you have a mixture of the two in the same kernel
depending on what instance of the UART you are.

regards,

Peter.

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

On Mon, 30 Jan 2017, Peter Griffin wrote:
> On Mon, 30 Jan 2017, Lee Jones wrote:
> > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > > > 
> > > > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > > > 
> > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > ---
> > > > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > > > index 397df50..03801ed 100644
> > > > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > > > +++ b/drivers/tty/serial/st-asc.c
> > 
> > [...]
> > 
> > > > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > > > +
> > > > > > > 
> > > > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > > > 
> > > > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > > > 'default' pinmux config.
> > > > > > > 
> > > > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > > > 
> > > > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > > > to what is currently already the default for the other UARTS? And then make
> > > > > > > hw-flow-control the optional state for serial0?
> > > > > > > 
> > > > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > > > 
> > > > > > The reason it was done is this was because none of the other UARTs
> > > > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > > > if they support RTS/CTS then I believe that the lines should be
> > > > > > defined in Pinctrl.
> > > > > 
> > > > > Yes I agree with that.
> > > > > 
> > > > > > Thus, it was my plan to update all UART's default
> > > > > > Pinctrl configs to include the RTS/CTS lines.
> > > > > > 
> > > > > 
> > > > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > > > ABI if you don't need to?
> > > > > 
> > > > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > > > groups you get all the benefits of this series whilst also maintaining backwards
> > > > > compatbility with older DT's.
> > > > 
> > > > What makes you think this will break ABI?
> > > 
> > > I've not tried it, but an older DT defines one group, 'default' which contains
> > > the same pin config as your new optional 'manual-rts' group.
> > > 
> > > The driver now reads like the manual-rts pin config is optional and should be stored in
> > > ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> > > group and it will be stored in ascport->states[DEFAULT].
> > > 
> > > That seems wrong to me, and if it executes OK it wouldn't be what you
> > > expect by reading the code.
> > 
> > This makes no sense at a functional level.
> > 
> > Old kernel, old DTB:
> > 
> > ASC driver doesn't understand Pinctrl, but since only the "default"
> > state is defined, that's what will be used as a matter of course.
> > RTS/CTS aren't configured, but that doesn't matter because the DTS
> > does not advertise that HW flow-control is available.  In this
> > use-case neither HW flow-control, nor manual toggling of the RTS line
> > is possible.
> > 
> > New kernel, old DTB:
> > 
> > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > but "manual-rts" isn't available so "default" will be the only
> > utilised state.  Unlike the first example above, "default" now
> > contains the RTS and CTS lines,
> 
> No it doesn't, default just contains 'tx' & 'rx' pins, as it has always
> done until now.
> 
> Which is IMO where the condusion arises, as it is the same pin configuration
> as what you are now calling 'manual-rts' which the driver just tried and failed
> to obtain (although in reality it has actually obtained those pins but stored
> them in DEFAULT instead.
> 
> I presume this is why it didn't make sense to you above.

I guess this is what happens when you try to explain semantics last
thing, after a long day at work.  I chopped and changed the
descriptions and the ordering of these and it looks like some
peculiarities arose as a result.  Let me try again with a fresh(ish)
mind.

New kernel, old DTB:

ASC driver demands "default" and requests "manual-rts" Pinctrl states,
but "manual-rts" isn't available so "default" will be the only
utilised state.  The RTS and CTS lines will not be present, but since
the DTB is not advertising HW flow-control as a possibility, the IP
will not try to use those lines anyway.  [DEFAULT] will contain the
"default" state as proposed by the current DTB, so that is also
semantically correct.

> >but since the DTS does not advertise
> > HW flow-control as available they will be harmlessly unused.  This
> > configuration culminates in the same result as the first example
> > i.e. no HW flow-control and no manual toggling.  However, there are no
> > detremental effects to the driver's functions. 
> >
> 
> <snip>
> 
> >New kernel, new DTB:
> > 
> > ASC driver demands "default" and requests "manual-rts" Pinctrl
> > states.  If DTS advertises that HW flow-control is possible and the
> > client requests it, ASC will use the "default" state and HW
> > flow-control will commence.  If HW flow-control is not requested by
> > the client and "manual-rts" is available, then ASC will request the
> > RTS line is handled by GPIO until such times as the client requests
> > HW flow-control, at which point ASC will disable GPIO and request the
> > "default" state again.
> 
> Unless it is uart 1 or 2, in which case 'default' still only contains
> tx & rx pins, and you have the same situation as above. 

Doesn't matter.  "default" is non-descriptive.  I could understand an
argument were you to say that the "manual-rts" should not contain a
non-manual-rts state, but the "default" state should just contain
whatever the default configuration is, and in the case of UART 1 and
UART 2 the default state (until they are HW flow-control enabled --
which I plan to do as a follow-up) is not to provide HW flow-control
pins.  These semantics are unchanged since authorship of the driver.

> > It is not possible to read C-code and make assumptions that the DTB
> > will be in a particular state as you suggest.
> > No disparity ever
> > exists and the code is always clear IMHO.
> > 
> 
> Really?

Yes.

> ascport->states[DEFAULT]: may contain "tx, rx" or "tx, rx, cts & rts"

Correct.  "DEFAULT" does not mean "HW flow-control only".  It's
whatever the default is, so can correctly contain either state,
depending on what the default state of the DTB is.

> ascport->states[MANUAL_RTS]: may contain "tx, rx", or it could be stored in DEFAULT

The last part of this is reiterating your previous point, which I
just answered.  The correct description would be; "may contain *only*
"tx, rx", allowing "rts" to be manually controlled OR, may not be
populated".  In the latter case it would not be semantically incorrect
for DEFAULT to be either HW flow-control capable "tx, rx, rts, cts" or
not "tx, rx" -- whichever is the default of the supplied DTB.

> And as the series currently is you have a mixture of the two in the same kernel
> depending on what instance of the UART you are.

Again, doesn't matter, since it's the DTB that provides the default
state.  So, back when it was authored, the default state was HW
flow-control disabled.  And in a newer DTB (again, until I follow-up
with more changes), the defaults for UART 1 and UART 2 are HW
flow-control disabled.

Your issue seems to be that you've assumed since we now provide the
possibility of a "manual-rts" state, then the "default" state should
*only* be HW flow-control capable, which is not the case.  It's the
'uart-has-rtscts' property which determines this *not* whether the
second state has been provided.  It is not logical to make any
inference using solely the presence or absence of the "manual-rts"
state.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Tue, 31 Jan 2017, Lee Jones wrote:

> On Mon, 30 Jan 2017, Peter Griffin wrote:
> > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > > > > 
> > > > > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > > > > index 397df50..03801ed 100644
> > > > > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > > > > +++ b/drivers/tty/serial/st-asc.c
> > > 
> > > [...]
> > > 
> > > > > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > > > > 
> > > > > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > > > > 'default' pinmux config.
> > > > > > > > 
> > > > > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > > > > 
> > > > > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > > > > to what is currently already the default for the other UARTS? And then make
> > > > > > > > hw-flow-control the optional state for serial0?
> > > > > > > > 
> > > > > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > > > > 
> > > > > > > The reason it was done is this was because none of the other UARTs
> > > > > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > > > > if they support RTS/CTS then I believe that the lines should be
> > > > > > > defined in Pinctrl.
> > > > > > 
> > > > > > Yes I agree with that.
> > > > > > 
> > > > > > > Thus, it was my plan to update all UART's default
> > > > > > > Pinctrl configs to include the RTS/CTS lines.
> > > > > > > 
> > > > > > 
> > > > > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > > > > ABI if you don't need to?
> > > > > > 
> > > > > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > > > > groups you get all the benefits of this series whilst also maintaining backwards
> > > > > > compatbility with older DT's.
> > > > > 
> > > > > What makes you think this will break ABI?
> > > > 
> > > > I've not tried it, but an older DT defines one group, 'default' which contains
> > > > the same pin config as your new optional 'manual-rts' group.
> > > > 
> > > > The driver now reads like the manual-rts pin config is optional and should be stored in
> > > > ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> > > > group and it will be stored in ascport->states[DEFAULT].
> > > > 
> > > > That seems wrong to me, and if it executes OK it wouldn't be what you
> > > > expect by reading the code.
> > > 
> > > This makes no sense at a functional level.
> > > 
> > > Old kernel, old DTB:
> > > 
> > > ASC driver doesn't understand Pinctrl, but since only the "default"
> > > state is defined, that's what will be used as a matter of course.
> > > RTS/CTS aren't configured, but that doesn't matter because the DTS
> > > does not advertise that HW flow-control is available.  In this
> > > use-case neither HW flow-control, nor manual toggling of the RTS line
> > > is possible.
> > > 
> > > New kernel, old DTB:
> > > 
> > > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > > but "manual-rts" isn't available so "default" will be the only
> > > utilised state.  Unlike the first example above, "default" now
> > > contains the RTS and CTS lines,
> > 
> > No it doesn't, default just contains 'tx' & 'rx' pins, as it has always
> > done until now.
> > 
> > Which is IMO where the condusion arises, as it is the same pin configuration
> > as what you are now calling 'manual-rts' which the driver just tried and failed
> > to obtain (although in reality it has actually obtained those pins but stored
> > them in DEFAULT instead.
> > 
> > I presume this is why it didn't make sense to you above.
> 
> I guess this is what happens when you try to explain semantics last
> thing, after a long day at work.  I chopped and changed the
> descriptions and the ordering of these and it looks like some
> peculiarities arose as a result.  Let me try again with a fresh(ish)
> mind.

It is what happens when your semantics are overly complicated ;)

> 
> New kernel, old DTB:
> 
> ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> but "manual-rts" isn't available so "default" will be the only
> utilised state.  The RTS and CTS lines will not be present, but since
> the DTB is not advertising HW flow-control as a possibility, the IP
> will not try to use those lines anyway.  [DEFAULT] will contain the
> "default" state as proposed by the current DTB, so that is also
> semantically correct.
> 
> > >but since the DTS does not advertise
> > > HW flow-control as available they will be harmlessly unused.  This
> > > configuration culminates in the same result as the first example
> > > i.e. no HW flow-control and no manual toggling.  However, there are no
> > > detremental effects to the driver's functions. 
> > >
> > 
> > <snip>
> > 
> > >New kernel, new DTB:
> > > 
> > > ASC driver demands "default" and requests "manual-rts" Pinctrl
> > > states.  If DTS advertises that HW flow-control is possible and the
> > > client requests it, ASC will use the "default" state and HW
> > > flow-control will commence.  If HW flow-control is not requested by
> > > the client and "manual-rts" is available, then ASC will request the
> > > RTS line is handled by GPIO until such times as the client requests
> > > HW flow-control, at which point ASC will disable GPIO and request the
> > > "default" state again.
> > 
> > Unless it is uart 1 or 2, in which case 'default' still only contains
> > tx & rx pins, and you have the same situation as above. 
> 
> Doesn't matter. "default" is non-descriptive.  I could understand an
> argument were you to say that the "manual-rts" should not contain a
> non-manual-rts state, but the "default" state should just contain
> whatever the default configuration is, and in the case of UART 1 and
> UART 2 the default state (until they are HW flow-control enabled --
> which I plan to do as a follow-up) is not to provide HW flow-control
> pins.  These semantics are unchanged since authorship of the driver.
> 
> > > It is not possible to read C-code and make assumptions that the DTB
> > > will be in a particular state as you suggest.
> > > No disparity ever
> > > exists and the code is always clear IMHO.
> > > 
> > 
> > Really?
> 
> Yes.
> 
> > ascport->states[DEFAULT]: may contain "tx, rx" or "tx, rx, cts & rts"
> 
> Correct.  "DEFAULT" does not mean "HW flow-control only".  It's
> whatever the default is, so can correctly contain either state,
> depending on what the default state of the DTB is.
> 
> > ascport->states[MANUAL_RTS]: may contain "tx, rx", or it could be stored in DEFAULT
> 
> The last part of this is reiterating your previous point, which I
> just answered.  The correct description would be; "may contain *only*
> "tx, rx", allowing "rts" to be manually controlled OR, may not be
> populated".  In the latter case it would not be semantically incorrect
> for DEFAULT to be either HW flow-control capable "tx, rx, rts, cts" or
> not "tx, rx" -- whichever is the default of the supplied DTB.
> 
> > And as the series currently is you have a mixture of the two in the same kernel
> > depending on what instance of the UART you are.
> 
> Again, doesn't matter, since it's the DTB that provides the default
> state.  So, back when it was authored, the default state was HW
> flow-control disabled.  And in a newer DTB (again, until I follow-up
> with more changes), the defaults for UART 1 and UART 2 are HW
> flow-control disabled.
> 
> Your issue seems to be that you've assumed since we now provide the
> possibility of a "manual-rts" state, then the "default" state should
> *only* be HW flow-control capable, which is not the case.

No my feedback was that it would be clearer & simpler to make manual-rts the
'default' state, and 'hw-flow-control' the optional state.

> It's the
> 'uart-has-rtscts' property which determines this *not* whether the
> second state has been provided.

Yep, which is why IMO it makes more sense for the optional pin group to be the hw
flow control pins which are obtained if the uart-has-rtscts property is present.

>It is not logical to make any
> inference using solely the presence or absence of the "manual-rts"
> state.

My suggestion would mean 'default' continues to mean 'tx & rx' pins, and the presence
of 'uart-has-rtscts' would mean the driver attempts to obtain a hw-flow-control
pin group.

In this setup

ascport->states[DEFAULT]: always contains tx, rx
ascport->states[HWFLOW]: always contains tx, rx, cts, rts or nothing

and the presence or lack of rts-gpio controls manual RTS toggling. This seems
simpler than your current semantics, and still understandable even late at night
after a long day :)

regards,

Peter.

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

On Tue, 31 Jan 2017, Peter Griffin wrote:
> On Tue, 31 Jan 2017, Lee Jones wrote:
> > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > > > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > > > > > 
> > > > > > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > > > > > index 397df50..03801ed 100644
> > > > > > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > > > > > +++ b/drivers/tty/serial/st-asc.c
> > > > 
> > > > [...]
> > > > 
> > > > > > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > > > > > +
> > > > > > > > > 
> > > > > > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > > > > > 
> > > > > > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > > > > > 'default' pinmux config.
> > > > > > > > > 
> > > > > > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > > > > > 
> > > > > > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > > > > > to what is currently already the default for the other UARTS? And then make
> > > > > > > > > hw-flow-control the optional state for serial0?
> > > > > > > > > 
> > > > > > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > > > > > 
> > > > > > > > The reason it was done is this was because none of the other UARTs
> > > > > > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > > > > > if they support RTS/CTS then I believe that the lines should be
> > > > > > > > defined in Pinctrl.
> > > > > > > 
> > > > > > > Yes I agree with that.
> > > > > > > 
> > > > > > > > Thus, it was my plan to update all UART's default
> > > > > > > > Pinctrl configs to include the RTS/CTS lines.
> > > > > > > > 
> > > > > > > 
> > > > > > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > > > > > ABI if you don't need to?
> > > > > > > 
> > > > > > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > > > > > groups you get all the benefits of this series whilst also maintaining backwards
> > > > > > > compatbility with older DT's.
> > > > > > 
> > > > > > What makes you think this will break ABI?
> > > > > 
> > > > > I've not tried it, but an older DT defines one group, 'default' which contains
> > > > > the same pin config as your new optional 'manual-rts' group.
> > > > > 
> > > > > The driver now reads like the manual-rts pin config is optional and should be stored in
> > > > > ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> > > > > group and it will be stored in ascport->states[DEFAULT].
> > > > > 
> > > > > That seems wrong to me, and if it executes OK it wouldn't be what you
> > > > > expect by reading the code.
> > > > 
> > > > This makes no sense at a functional level.
> > > > 
> > > > Old kernel, old DTB:
> > > > 
> > > > ASC driver doesn't understand Pinctrl, but since only the "default"
> > > > state is defined, that's what will be used as a matter of course.
> > > > RTS/CTS aren't configured, but that doesn't matter because the DTS
> > > > does not advertise that HW flow-control is available.  In this
> > > > use-case neither HW flow-control, nor manual toggling of the RTS line
> > > > is possible.
> > > > 
> > > > New kernel, old DTB:
> > > > 
> > > > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > > > but "manual-rts" isn't available so "default" will be the only
> > > > utilised state.  Unlike the first example above, "default" now
> > > > contains the RTS and CTS lines,
> > > 
> > > No it doesn't, default just contains 'tx' & 'rx' pins, as it has always
> > > done until now.
> > > 
> > > Which is IMO where the condusion arises, as it is the same pin configuration
> > > as what you are now calling 'manual-rts' which the driver just tried and failed
> > > to obtain (although in reality it has actually obtained those pins but stored
> > > them in DEFAULT instead.
> > > 
> > > I presume this is why it didn't make sense to you above.
> > 
> > I guess this is what happens when you try to explain semantics last
> > thing, after a long day at work.  I chopped and changed the
> > descriptions and the ordering of these and it looks like some
> > peculiarities arose as a result.  Let me try again with a fresh(ish)
> > mind.
> 
> It is what happens when your semantics are overly complicated ;)

> [...]  and still understandable even late at night after a long day :)

[0]

It's not that I didn't understand the semantics.  It's that I left the 
wrong description in when cutting my text around.  It's English I have
a problem with late at night, not understanding this simple concept. ;)

> > New kernel, old DTB:
> > 
> > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > but "manual-rts" isn't available so "default" will be the only
> > utilised state.  The RTS and CTS lines will not be present, but since
> > the DTB is not advertising HW flow-control as a possibility, the IP
> > will not try to use those lines anyway.  [DEFAULT] will contain the
> > "default" state as proposed by the current DTB, so that is also
> > semantically correct.
> > 
> > > >but since the DTS does not advertise
> > > > HW flow-control as available they will be harmlessly unused.  This
> > > > configuration culminates in the same result as the first example
> > > > i.e. no HW flow-control and no manual toggling.  However, there are no
> > > > detremental effects to the driver's functions. 
> > > >
> > > 
> > > <snip>
> > > 
> > > >New kernel, new DTB:
> > > > 
> > > > ASC driver demands "default" and requests "manual-rts" Pinctrl
> > > > states.  If DTS advertises that HW flow-control is possible and the
> > > > client requests it, ASC will use the "default" state and HW
> > > > flow-control will commence.  If HW flow-control is not requested by
> > > > the client and "manual-rts" is available, then ASC will request the
> > > > RTS line is handled by GPIO until such times as the client requests
> > > > HW flow-control, at which point ASC will disable GPIO and request the
> > > > "default" state again.
> > > 
> > > Unless it is uart 1 or 2, in which case 'default' still only contains
> > > tx & rx pins, and you have the same situation as above. 
> > 
> > Doesn't matter. "default" is non-descriptive.  I could understand an
> > argument were you to say that the "manual-rts" should not contain a
> > non-manual-rts state, but the "default" state should just contain
> > whatever the default configuration is, and in the case of UART 1 and
> > UART 2 the default state (until they are HW flow-control enabled --
> > which I plan to do as a follow-up) is not to provide HW flow-control
> > pins.  These semantics are unchanged since authorship of the driver.
> > 
> > > > It is not possible to read C-code and make assumptions that the DTB
> > > > will be in a particular state as you suggest.
> > > > No disparity ever
> > > > exists and the code is always clear IMHO.
> > > > 
> > > 
> > > Really?
> > 
> > Yes.
> > 
> > > ascport->states[DEFAULT]: may contain "tx, rx" or "tx, rx, cts & rts"
> > 
> > Correct.  "DEFAULT" does not mean "HW flow-control only".  It's
> > whatever the default is, so can correctly contain either state,
> > depending on what the default state of the DTB is.
> > 
> > > ascport->states[MANUAL_RTS]: may contain "tx, rx", or it could be stored in DEFAULT
> > 
> > The last part of this is reiterating your previous point, which I
> > just answered.  The correct description would be; "may contain *only*
> > "tx, rx", allowing "rts" to be manually controlled OR, may not be
> > populated".  In the latter case it would not be semantically incorrect
> > for DEFAULT to be either HW flow-control capable "tx, rx, rts, cts" or
> > not "tx, rx" -- whichever is the default of the supplied DTB.
> > 
> > > And as the series currently is you have a mixture of the two in the same kernel
> > > depending on what instance of the UART you are.
> > 
> > Again, doesn't matter, since it's the DTB that provides the default
> > state.  So, back when it was authored, the default state was HW
> > flow-control disabled.  And in a newer DTB (again, until I follow-up
> > with more changes), the defaults for UART 1 and UART 2 are HW
> > flow-control disabled.
> > 
> > Your issue seems to be that you've assumed since we now provide the
> > possibility of a "manual-rts" state, then the "default" state should
> > *only* be HW flow-control capable, which is not the case.
> 
> No my feedback was that it would be clearer & simpler to make manual-rts the
> 'default' state, and 'hw-flow-control' the optional state.

Absolutely not.  The use of "manual-rts" is the corner-case here and
is not normally required.  The "default" state should normally be
populated with whatever pins are available i.e. all 4 pins (including
"rts, cts") if they are wired up and only 2 pins (just "tx, rx") if
they are not.  The submission of "manual-rts" is only required if the
RTS pin is required for some other purpose e.g. resetting a uC on a
draughtboard.

> > It's the
> > 'uart-has-rtscts' property which determines this *not* whether the
> > second state has been provided.
> 
> Yep, which is why IMO it makes more sense for the optional pin group to be the hw
> flow control pins which are obtained if the uart-has-rtscts property is present.

There would normally only be one pin group.  Your method would insist
we always provided 2, which would be surplus to requirement.

> >It is not logical to make any
> > inference using solely the presence or absence of the "manual-rts"
> > state.
> 
> My suggestion would mean 'default' continues to mean 'tx & rx' pins, and the presence
> of 'uart-has-rtscts' would mean the driver attempts to obtain a hw-flow-control
> pin group.
> 
> In this setup
> 
> ascport->states[DEFAULT]: always contains tx, rx
> ascport->states[HWFLOW]: always contains tx, rx, cts, rts or nothing

I know what your suggestion would mean, and I think it's hacky.
"default" means default, whatever that may be.  We should have to a)
provide one Pinctrl if only one is required (most cases) and b) make
assumptions based solely on the presence/absence of the
'uart-has-rtscts' property and nothing else.

There is nothing complicated about that.

> and the presence or lack of rts-gpio controls manual RTS toggling. This seems
> simpler than your current semantics, and still understandable even late at night
> after a long day :)

I don't think your method makes it simpler at all.  I think it makes
illogical assumptions (there is no reason why "default" should mean
"HW flow-control is disabled") and forces us to over-complicate the DTB.

For the second part, see [0] above. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Peter Griffin]

Hi Lee,

On Tue, 31 Jan 2017, Lee Jones wrote:

> On Tue, 31 Jan 2017, Peter Griffin wrote:
> > On Tue, 31 Jan 2017, Lee Jones wrote:
> > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > > On Mon, 30 Jan 2017, Lee Jones wrote:
> > > > > > > On Mon, 30 Jan 2017, Peter Griffin wrote:
> > > > > > > > On Fri, 27 Jan 2017, Lee Jones wrote:
> > > > > > > > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > > > > > > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > > > > > > > 
> > > > > > > > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > > > > > > > be provided from platform data.  One which encompasses the lines
> > > > > > > > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > > > > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > > > > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > > > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > > > > > > > index 397df50..03801ed 100644
> > > > > > > > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > > > > > > > +++ b/drivers/tty/serial/st-asc.c
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > > > > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > > > > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > > > > > > > +
> > > > > > > > > > 
> > > > > > > > > > The different pinctrl states looks like a neat solution to the problem.
> > > > > > > > > > 
> > > > > > > > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > > > > > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > > > > > > > 'default' pinmux config.
> > > > > > > > > > 
> > > > > > > > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > > > > > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > > > > > > > 'manual-rts' grouping, which conceptually is odd.
> > > > > > > > > > 
> > > > > > > > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > > > > > > > to what is currently already the default for the other UARTS? And then make
> > > > > > > > > > hw-flow-control the optional state for serial0?
> > > > > > > > > > 
> > > > > > > > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > > > > > > > 
> > > > > > > > > The reason it was done is this was because none of the other UARTs
> > > > > > > > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > > > > > > > if they support RTS/CTS then I believe that the lines should be
> > > > > > > > > defined in Pinctrl.
> > > > > > > > 
> > > > > > > > Yes I agree with that.
> > > > > > > > 
> > > > > > > > > Thus, it was my plan to update all UART's default
> > > > > > > > > Pinctrl configs to include the RTS/CTS lines.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I still don't see the point in changing the meaning of 'default' group and breaking
> > > > > > > > ABI if you don't need to?
> > > > > > > > 
> > > > > > > > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > > > > > > > groups you get all the benefits of this series whilst also maintaining backwards
> > > > > > > > compatbility with older DT's.
> > > > > > > 
> > > > > > > What makes you think this will break ABI?
> > > > > > 
> > > > > > I've not tried it, but an older DT defines one group, 'default' which contains
> > > > > > the same pin config as your new optional 'manual-rts' group.
> > > > > > 
> > > > > > The driver now reads like the manual-rts pin config is optional and should be stored in
> > > > > > ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
> > > > > > group and it will be stored in ascport->states[DEFAULT].
> > > > > > 
> > > > > > That seems wrong to me, and if it executes OK it wouldn't be what you
> > > > > > expect by reading the code.
> > > > > 
> > > > > This makes no sense at a functional level.
> > > > > 
> > > > > Old kernel, old DTB:
> > > > > 
> > > > > ASC driver doesn't understand Pinctrl, but since only the "default"
> > > > > state is defined, that's what will be used as a matter of course.
> > > > > RTS/CTS aren't configured, but that doesn't matter because the DTS
> > > > > does not advertise that HW flow-control is available.  In this
> > > > > use-case neither HW flow-control, nor manual toggling of the RTS line
> > > > > is possible.
> > > > > 
> > > > > New kernel, old DTB:
> > > > > 
> > > > > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > > > > but "manual-rts" isn't available so "default" will be the only
> > > > > utilised state.  Unlike the first example above, "default" now
> > > > > contains the RTS and CTS lines,
> > > > 
> > > > No it doesn't, default just contains 'tx' & 'rx' pins, as it has always
> > > > done until now.
> > > > 
> > > > Which is IMO where the condusion arises, as it is the same pin configuration
> > > > as what you are now calling 'manual-rts' which the driver just tried and failed
> > > > to obtain (although in reality it has actually obtained those pins but stored
> > > > them in DEFAULT instead.
> > > > 
> > > > I presume this is why it didn't make sense to you above.
> > > 
> > > I guess this is what happens when you try to explain semantics last
> > > thing, after a long day at work.  I chopped and changed the
> > > descriptions and the ordering of these and it looks like some
> > > peculiarities arose as a result.  Let me try again with a fresh(ish)
> > > mind.
> > 
> > It is what happens when your semantics are overly complicated ;)
> 
> > [...]  and still understandable even late at night after a long day :)
> 
> [0]
> 
> It's not that I didn't understand the semantics.  It's that I left the 
> wrong description in when cutting my text around.  It's English I have
> a problem with late at night, not understanding this simple concept. ;)
> 
> > > New kernel, old DTB:
> > > 
> > > ASC driver demands "default" and requests "manual-rts" Pinctrl states,
> > > but "manual-rts" isn't available so "default" will be the only
> > > utilised state.  The RTS and CTS lines will not be present, but since
> > > the DTB is not advertising HW flow-control as a possibility, the IP
> > > will not try to use those lines anyway.  [DEFAULT] will contain the
> > > "default" state as proposed by the current DTB, so that is also
> > > semantically correct.
> > > 
> > > > >but since the DTS does not advertise
> > > > > HW flow-control as available they will be harmlessly unused.  This
> > > > > configuration culminates in the same result as the first example
> > > > > i.e. no HW flow-control and no manual toggling.  However, there are no
> > > > > detremental effects to the driver's functions. 
> > > > >
> > > > 
> > > > <snip>
> > > > 
> > > > >New kernel, new DTB:
> > > > > 
> > > > > ASC driver demands "default" and requests "manual-rts" Pinctrl
> > > > > states.  If DTS advertises that HW flow-control is possible and the
> > > > > client requests it, ASC will use the "default" state and HW
> > > > > flow-control will commence.  If HW flow-control is not requested by
> > > > > the client and "manual-rts" is available, then ASC will request the
> > > > > RTS line is handled by GPIO until such times as the client requests
> > > > > HW flow-control, at which point ASC will disable GPIO and request the
> > > > > "default" state again.
> > > > 
> > > > Unless it is uart 1 or 2, in which case 'default' still only contains
> > > > tx & rx pins, and you have the same situation as above. 
> > > 
> > > Doesn't matter. "default" is non-descriptive.  I could understand an
> > > argument were you to say that the "manual-rts" should not contain a
> > > non-manual-rts state, but the "default" state should just contain
> > > whatever the default configuration is, and in the case of UART 1 and
> > > UART 2 the default state (until they are HW flow-control enabled --
> > > which I plan to do as a follow-up) is not to provide HW flow-control
> > > pins.  These semantics are unchanged since authorship of the driver.
> > > 
> > > > > It is not possible to read C-code and make assumptions that the DTB
> > > > > will be in a particular state as you suggest.
> > > > > No disparity ever
> > > > > exists and the code is always clear IMHO.
> > > > > 
> > > > 
> > > > Really?
> > > 
> > > Yes.
> > > 
> > > > ascport->states[DEFAULT]: may contain "tx, rx" or "tx, rx, cts & rts"
> > > 
> > > Correct.  "DEFAULT" does not mean "HW flow-control only".  It's
> > > whatever the default is, so can correctly contain either state,
> > > depending on what the default state of the DTB is.
> > > 
> > > > ascport->states[MANUAL_RTS]: may contain "tx, rx", or it could be stored in DEFAULT
> > > 
> > > The last part of this is reiterating your previous point, which I
> > > just answered.  The correct description would be; "may contain *only*
> > > "tx, rx", allowing "rts" to be manually controlled OR, may not be
> > > populated".  In the latter case it would not be semantically incorrect
> > > for DEFAULT to be either HW flow-control capable "tx, rx, rts, cts" or
> > > not "tx, rx" -- whichever is the default of the supplied DTB.
> > > 
> > > > And as the series currently is you have a mixture of the two in the same kernel
> > > > depending on what instance of the UART you are.
> > > 
> > > Again, doesn't matter, since it's the DTB that provides the default
> > > state.  So, back when it was authored, the default state was HW
> > > flow-control disabled.  And in a newer DTB (again, until I follow-up
> > > with more changes), the defaults for UART 1 and UART 2 are HW
> > > flow-control disabled.
> > > 
> > > Your issue seems to be that you've assumed since we now provide the
> > > possibility of a "manual-rts" state, then the "default" state should
> > > *only* be HW flow-control capable, which is not the case.
> > 
> > No my feedback was that it would be clearer & simpler to make manual-rts the
> > 'default' state, and 'hw-flow-control' the optional state.
> 
> Absolutely not.  The use of "manual-rts" is the corner-case here and
> is not normally required.

See below.

> The "default" state should normally be
> populated with whatever pins are available i.e. all 4 pins (including
> "rts, cts") if they are wired up and only 2 pins (just "tx, rx") if
> they are not.

Yep OK, I agree :)

> The submission of "manual-rts" is only required if the
> RTS pin is required for some other purpose e.g. resetting a uC on a
> draughtboard.

All UARTs the SoC have the same st-asc IP, which suffers from the same
hw flow control limitation. Also all instances on the SoC have rts/cts
pins, the only limitation is board wiring.

So I can't see why would you ever *not* want to deploy this dynamic pin
switching solution if rts/cts is wired up at board level now the facility
exists?

Also regarding the naming of the second pin group, 'manual-rts' seems like
a bad name as a logical extension of this set is to also offer the same
dynamic switching for the CTS line.

Maybe a better name would be 'tx-rx-only' or 'no-rts-cts'.

> 
> > > It's the
> > > 'uart-has-rtscts' property which determines this *not* whether the
> > > second state has been provided.
> > 
> > Yep, which is why IMO it makes more sense for the optional pin group to be the hw
> > flow control pins which are obtained if the uart-has-rtscts property is present.
> 
> There would normally only be one pin group.  Your method would insist
> we always provided 2, which would be surplus to requirement.

Yep OK, agree with your point.

> > >It is not logical to make any
> > > inference using solely the presence or absence of the "manual-rts"
> > > state.
> > 
> > My suggestion would mean 'default' continues to mean 'tx & rx' pins, and the presence
> > of 'uart-has-rtscts' would mean the driver attempts to obtain a hw-flow-control
> > pin group.
> > 
> > In this setup
> > 
> > ascport->states[DEFAULT]: always contains tx, rx
> > ascport->states[HWFLOW]: always contains tx, rx, cts, rts or nothing
> 
> I know what your suggestion would mean, and I think it's hacky.
> "default" means default, whatever that may be.  We should have to a)
> provide one Pinctrl if only one is required (most cases) and b) make
> assumptions based solely on the presence/absence of the
> 'uart-has-rtscts' property and nothing else.
> 
> There is nothing complicated about that.

Yep OK, I agree.

> 
> > and the presence or lack of rts-gpio controls manual RTS toggling. This seems
> > simpler than your current semantics, and still understandable even late at night
> > after a long day :)
> 
> I don't think your method makes it simpler at all.  I think it makes
> illogical assumptions (there is no reason why "default" should mean
> "HW flow-control is disabled") and forces us to over-complicate the DTB.

Yep OK, I agree.

> 
> For the second part, see [0] above. :)
> 
regards,

Peter.

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

[Lee Jones]

> > > > Again, doesn't matter, since it's the DTB that provides the default
> > > > state.  So, back when it was authored, the default state was HW
> > > > flow-control disabled.  And in a newer DTB (again, until I follow-up
> > > > with more changes), the defaults for UART 1 and UART 2 are HW
> > > > flow-control disabled.
> > > > 
> > > > Your issue seems to be that you've assumed since we now provide the
> > > > possibility of a "manual-rts" state, then the "default" state should
> > > > *only* be HW flow-control capable, which is not the case.
> > > 
> > > No my feedback was that it would be clearer & simpler to make manual-rts the
> > > 'default' state, and 'hw-flow-control' the optional state.
> > 
> > Absolutely not.  The use of "manual-rts" is the corner-case here and
> > is not normally required.
> 
> See below.
> 
> > The "default" state should normally be
> > populated with whatever pins are available i.e. all 4 pins (including
> > "rts, cts") if they are wired up and only 2 pins (just "tx, rx") if
> > they are not.
> 
> Yep OK, I agree :)

\o/

> > The submission of "manual-rts" is only required if the
> > RTS pin is required for some other purpose e.g. resetting a uC on a
> > draughtboard.
> 
> All UARTs the SoC have the same st-asc IP, which suffers from the same
> hw flow control limitation. Also all instances on the SoC have rts/cts
> pins, the only limitation is board wiring.
> 
> So I can't see why would you ever *not* want to deploy this dynamic pin
> switching solution if rts/cts is wired up at board level now the facility
> exists?

Mainly because it's surplus to requirement, in that there is very
seldom any point in manually toggling the RTS line (at least to my
knowledge).  I figure we'd add >1 Pinctrl states only when the need
arises, thus keeping the DTS' as simple as possible.

> Also regarding the naming of the second pin group, 'manual-rts' seems like
> a bad name as a logical extension of this set is to also offer the same
> dynamic switching for the CTS line.
> 
> Maybe a better name would be 'tx-rx-only' or 'no-rts-cts'.

Works for me.  Will fix.

> > > > It's the
> > > > 'uart-has-rtscts' property which determines this *not* whether the
> > > > second state has been provided.
> > > 
> > > Yep, which is why IMO it makes more sense for the optional pin group to be the hw
> > > flow control pins which are obtained if the uart-has-rtscts property is present.
> > 
> > There would normally only be one pin group.  Your method would insist
> > we always provided 2, which would be surplus to requirement.
> 
> Yep OK, agree with your point.

\o/

> Yep OK, I agree.

\o/

> Yep OK, I agree.

\o/

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog