[PATCH/RFC 0/5] serial: sh-sci: Hardware Flow Control Updates

[PATCH/RFC 0/5] serial: sh-sci: Hardware Flow Control Updates

[Geert Uytterhoeven]

	Hi all,

This RFC patch series contains updates to the Renesas SCI UART driver,
related to hardware flow control:
  - Device Tree binding updates for GPIO-controlled modem lines, and for
    dedicated RTS/CTS modem lines,
  - Driver support for GPIO-controlled modem lines, using the
    SERIAL_MCTRL_GPIO helpers,
  - Some preparations for fixing hardware-assisted flow control using
    the dedicated RTS/CTS modem lines later.

This was tested on r8a7791/koelsch, using UART and GPIO pins on expansion
connectors (I've pushed a few more DT overlays for testing to the
topic/renesas-overlays branch of
https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git)

Testing for regressions on platforms without DT and/or GPIOLIB support
(SuperH) would be appreciated.
Compile-tested on ecovec24_defconfig(GPIOLIB=y) and se7780_defconfig
(GPIOLIB=n).

Thanks for your comments!

Geert Uytterhoeven (5):
  serial: sh-sci: Update DT binding documentation for GPIO modem lines
  serial: sh-sci: Update DT binding documentation for dedicated RTS/CTS
  serial: sh-sci: Always set TIOCM_CTS in .get_mctrl() callback
  serial: sh-sci: Add support for GPIO-controlled modem lines
  serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

 .../bindings/serial/renesas,sci-serial.txt         |  8 +++++
 drivers/tty/serial/Kconfig                         |  1 +
 drivers/tty/serial/sh-sci.c                        | 41 +++++++++++++++++++---
 include/linux/serial_sci.h                         |  6 ----
 4 files changed, 46 insertions(+), 10 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH/RFC 1/5] serial: sh-sci: Update DT binding documentation for GPIO modem lines

[Geert Uytterhoeven]

Amend the DT bindings for the Renesas SCI driver to allow describing
optional GPIO-controlled modem lines, which can be used where dedicated
modem lines are not available.

The property naming is dictated by the SERIAL_MCTRL_GPIO helpers.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
index 528c3b90f23cb04b..f8d7b36742967163 100644
--- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
+++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
@@ -76,6 +76,9 @@ Optional properties:
   - dmas: Must contain a list of two references to DMA specifiers, one for
 	  transmission, and one for reception.
   - dma-names: Must contain a list of two DMA names, "tx" and "rx".
+  - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
+    referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
+    DTR, OUT1, or OUT2 line.
 
 Example:
 	aliases {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/5] serial: sh-sci: Update DT binding documentation for GPIO modem lines

[Simon Horman]

On Thu, Mar 17, 2016 at 02:47:25PM +0100, Geert Uytterhoeven wrote:
> Amend the DT bindings for the Renesas SCI driver to allow describing
> optional GPIO-controlled modem lines, which can be used where dedicated
> modem lines are not available.
> 
> The property naming is dictated by the SERIAL_MCTRL_GPIO helpers.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> index 528c3b90f23cb04b..f8d7b36742967163 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> @@ -76,6 +76,9 @@ Optional properties:
>    - dmas: Must contain a list of two references to DMA specifiers, one for
>  	  transmission, and one for reception.
>    - dma-names: Must contain a list of two DMA names, "tx" and "rx".
> +  - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
> +    referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
> +    DTR, OUT1, or OUT2 line.
>  
>  Example:
>  	aliases {

Hi Geert,

I do not feel strongly about this but I wonder if it
would make sense to update the example with the new properties.

Re: [PATCH/RFC 1/5] serial: sh-sci: Update DT binding documentation for GPIO modem lines

[Geert Uytterhoeven]

Hi Simon,

On Thu, Mar 17, 2016 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Mar 17, 2016 at 02:47:25PM +0100, Geert Uytterhoeven wrote:
>> Amend the DT bindings for the Renesas SCI driver to allow describing
>> optional GPIO-controlled modem lines, which can be used where dedicated
>> modem lines are not available.
>>
>> The property naming is dictated by the SERIAL_MCTRL_GPIO helpers.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: devicetree@vger.kernel.org
>> ---
>>  Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
>> index 528c3b90f23cb04b..f8d7b36742967163 100644
>> --- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
>> +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
>> @@ -76,6 +76,9 @@ Optional properties:
>>    - dmas: Must contain a list of two references to DMA specifiers, one for
>>         transmission, and one for reception.
>>    - dma-names: Must contain a list of two DMA names, "tx" and "rx".
>> +  - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
>> +    referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
>> +    DTR, OUT1, or OUT2 line.
>>
>>  Example:
>>       aliases {
>
> Hi Geert,
>
> I do not feel strongly about this but I wonder if it
> would make sense to update the example with the new properties.

These are meant to be added to the board .dts file, not to the .dtsi files.
I can add an example if you like, let's hope nobody copies it blindly to a
.dtsi file...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 1/5] serial: sh-sci: Update DT binding documentation for GPIO modem lines

[Rob Herring]

On Thu, Mar 17, 2016 at 02:47:25PM +0100, Geert Uytterhoeven wrote:
> Amend the DT bindings for the Renesas SCI driver to allow describing
> optional GPIO-controlled modem lines, which can be used where dedicated
> modem lines are not available.
> 
> The property naming is dictated by the SERIAL_MCTRL_GPIO helpers.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> index 528c3b90f23cb04b..f8d7b36742967163 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> @@ -76,6 +76,9 @@ Optional properties:
>    - dmas: Must contain a list of two references to DMA specifiers, one for
>  	  transmission, and one for reception.
>    - dma-names: Must contain a list of two DMA names, "tx" and "rx".
> +  - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
> +    referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
> +    DTR, OUT1, or OUT2 line.

It would be good to document these in a common location as at least some 
are already in use. Otherwise,

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 2/5] serial: sh-sci: Update DT binding documentation for dedicated RTS/CTS

[Geert Uytterhoeven]

Some Renesas SCIF UARTs have dedicated lines for RTS/CTS hardware flow
control.  Whether these lines exist depends on SoC and UART instance
inside the SoC.  Whether these lines can be used for hardware flow
control depends on board wiring.

Amend the DT bindings with an optional property to indicate that RTS/CTS
hardware flow control lines exist, and can be used as such.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: devicetree@vger.kernel.org
---
This has been mimicked after the "fsl,uart-has-rtscts" and
"sirf,uart-has-rtscts" properties.

However, as this is fairly generic, perhaps it should just be named
"uart-has-rtscts" instead?
---
 Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
index f8d7b36742967163..8de177c187536c68 100644
--- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
+++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
@@ -79,6 +79,11 @@ Optional properties:
   - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
     referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
     DTR, OUT1, or OUT2 line.
+  - renesas,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).
+    Note that this property is mutually-exclusive with "cts-gpios" and
+    "rts-gpios" above.
 
 Example:
 	aliases {
-- 
1.9.1

Re: [PATCH/RFC 2/5] serial: sh-sci: Update DT binding documentation for dedicated RTS/CTS

[Rob Herring]

On Thu, Mar 17, 2016 at 02:47:26PM +0100, Geert Uytterhoeven wrote:
> Some Renesas SCIF UARTs have dedicated lines for RTS/CTS hardware flow
> control.  Whether these lines exist depends on SoC and UART instance
> inside the SoC.  Whether these lines can be used for hardware flow
> control depends on board wiring.
> 
> Amend the DT bindings with an optional property to indicate that RTS/CTS
> hardware flow control lines exist, and can be used as such.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: devicetree@vger.kernel.org
> ---
> This has been mimicked after the "fsl,uart-has-rtscts" and
> "sirf,uart-has-rtscts" properties.
> 
> However, as this is fairly generic, perhaps it should just be named
> "uart-has-rtscts" instead?

Yes. And there are some other variations of properties to enable 
flow-control.

> ---
>  Documentation/devicetree/bindings/serial/renesas,sci-serial.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> index f8d7b36742967163..8de177c187536c68 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt
> @@ -79,6 +79,11 @@ Optional properties:
>    - {cts,dsr,dcd,rng,rts,dtr,out1,out2}-gpios: Must contain a GPIO specifier,
>      referring to the GPIO pin to be used as the UART's CTS, DSR, DCD, RNG, RTS,
>      DTR, OUT1, or OUT2 line.
> +  - renesas,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).
> +    Note that this property is mutually-exclusive with "cts-gpios" and
> +    "rts-gpios" above.
>  
>  Example:
>  	aliases {
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 3/5] serial: sh-sci: Always set TIOCM_CTS in .get_mctrl() callback

[Geert Uytterhoeven]

Documentation/serial/driver clearly states:

    If the port does not support CTS, DCD or DSR, the driver should
    indicate that the signal is permanently active.

Hence always set TIOCM_CTS, as we currently don't look at the CTS
hardware line state at all.

FWIW, this fixes the transmit path when hardware-assisted flow control
is enabled, and userspace enables CRTSCTS.
The receive path is still broken, as RTS is never asserted.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 0130feb069aee02f..135f836642ab1c5a 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1835,9 +1835,9 @@ static unsigned int sci_get_mctrl(struct uart_port *port)
 {
 	/*
 	 * CTS/RTS is handled in hardware when supported, while nothing
-	 * else is wired up. Keep it simple and simply assert DSR/CAR.
+	 * else is wired up. Keep it simple and simply assert CTS/DSR/CAR.
 	 */
-	return TIOCM_DSR | TIOCM_CAR;
+	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
 }
 
 static void sci_break_ctl(struct uart_port *port, int break_state)
-- 
1.9.1

[PATCH/RFC 4/5] serial: sh-sci: Add support for GPIO-controlled modem lines

[Geert Uytterhoeven]

Enhance the Renesas SCI UART driver to add support for GPIO-controlled
modem lines (CTS, DSR, DCD, RNG, RTS, DTR, OUT1, and OUT2), using the
SERIAL_MCTRL_GPIO helpers.

GPIO-controlled modem lines can be used where dedicated modem lines are
not available.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Testing for regressions on platforms without DT and/or GPIOLIB support
(SuperH) would be appreciated.
Compile-tested on ecovec24_defconfig(GPIOLIB=y) and se7780_defconfig
(GPIOLIB=n).
---
 drivers/tty/serial/Kconfig  |  1 +
 drivers/tty/serial/sh-sci.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 13d4ed6caac4a78b..ebf91bbfdf0f999d 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -735,6 +735,7 @@ config SERIAL_SH_SCI
 	tristate "SuperH SCI(F) serial port support"
 	depends on SUPERH || ARCH_RENESAS || H8300 || COMPILE_TEST
 	select SERIAL_CORE
+	select SERIAL_MCTRL_GPIO if GPIOLIB
 
 config SERIAL_SH_SCI_NR_UARTS
 	int "Maximum number of SCI(F) serial ports"
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 135f836642ab1c5a..6897100ed5197df3 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -57,6 +57,7 @@
 #include <asm/sh_bios.h>
 #endif
 
+#include "serial_mctrl_gpio.h"
 #include "sh-sci.h"
 
 /* Offsets into the sci_port->irqs array */
@@ -111,6 +112,7 @@ struct sci_port {
 	unsigned int		error_clear;
 	unsigned int		sampling_rate_mask;
 	resource_size_t		reg_size;
+	struct mctrl_gpios	*gpios;
 
 	/* Break timer */
 	struct timer_list	break_timer;
@@ -1829,15 +1831,36 @@ static void sci_set_mctrl(struct uart_port *port, unsigned int mctrl)
 					serial_port_in(port, SCFCR) |
 					SCFCR_LOOP);
 	}
+
+	mctrl_gpio_set(to_sci_port(port)->gpios, mctrl);
 }
 
 static unsigned int sci_get_mctrl(struct uart_port *port)
 {
+	unsigned int mctrl = 0;
+
+	mctrl_gpio_get(to_sci_port(port)->gpios, &mctrl);
+
 	/*
 	 * CTS/RTS is handled in hardware when supported, while nothing
 	 * else is wired up. Keep it simple and simply assert CTS/DSR/CAR.
 	 */
-	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
+	if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
+					       UART_GPIO_CTS)))
+		mctrl |= TIOCM_CTS;
+	if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
+					       UART_GPIO_DSR)))
+		mctrl |= TIOCM_DSR;
+	if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
+					       UART_GPIO_CAR)))
+		mctrl |= TIOCM_CAR;
+
+	return mctrl;
+}
+
+static void sci_enable_ms(struct uart_port *port)
+{
+	mctrl_gpio_enable_ms(to_sci_port(port)->gpios);
 }
 
 static void sci_break_ctl(struct uart_port *port, int break_state)
@@ -1899,6 +1922,8 @@ static void sci_shutdown(struct uart_port *port)
 
 	dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
 
+	mctrl_gpio_disable_ms(to_sci_port(port)->gpios);
+
 	spin_lock_irqsave(&port->lock, flags);
 	sci_stop_rx(port);
 	sci_stop_tx(port);
@@ -2300,6 +2325,9 @@ done:
 		sci_start_rx(port);
 
 	sci_port_disable(s);
+
+	if (UART_ENABLE_MS(port, termios->c_cflag))
+		sci_enable_ms(port);
 }
 
 static void sci_pm(struct uart_port *port, unsigned int state,
@@ -2425,6 +2453,7 @@ static struct uart_ops sci_uart_ops = {
 	.start_tx	= sci_start_tx,
 	.stop_tx	= sci_stop_tx,
 	.stop_rx	= sci_stop_rx,
+	.enable_ms	= sci_enable_ms,
 	.break_ctl	= sci_break_ctl,
 	.startup	= sci_startup,
 	.shutdown	= sci_shutdown,
@@ -2912,6 +2941,10 @@ static int sci_probe_single(struct platform_device *dev,
 	if (ret)
 		return ret;
 
+	sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
+	if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
+		return PTR_ERR(sciport->gpios);
+
 	ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
 	if (ret) {
 		sci_cleanup_single(sciport);
-- 
1.9.1

Re: [PATCH/RFC 4/5] serial: sh-sci: Add support for GPIO-controlled modem lines

[Geert Uytterhoeven]

On Thu, Mar 17, 2016 at 2:47 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -1829,15 +1831,36 @@ static void sci_set_mctrl(struct uart_port *port, unsigned int mctrl)
>                                         serial_port_in(port, SCFCR) |
>                                         SCFCR_LOOP);
>         }
> +
> +       mctrl_gpio_set(to_sci_port(port)->gpios, mctrl);
>  }
>
>  static unsigned int sci_get_mctrl(struct uart_port *port)
>  {
> +       unsigned int mctrl = 0;
> +
> +       mctrl_gpio_get(to_sci_port(port)->gpios, &mctrl);
> +
>         /*
>          * CTS/RTS is handled in hardware when supported, while nothing
>          * else is wired up. Keep it simple and simply assert CTS/DSR/CAR.
>          */
> -       return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> +       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
> +                                              UART_GPIO_CTS)))
> +               mctrl |= TIOCM_CTS;
> +       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
> +                                              UART_GPIO_DSR)))
> +               mctrl |= TIOCM_DSR;
> +       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(to_sci_port(port)->gpios,
> +                                              UART_GPIO_CAR)))

Oops, something went wrong during rebase before sending: that should
have been "UART_GPIO_DCD".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Geert Uytterhoeven]

Replace the custom SCIx_HAVE_RTSCTS flag in the
plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
the uart_port.flags and plat_sci_port.flags fields.
Remove the now unused plat_sci_port.capabilities field.
Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.

Note that currently nothing sets the SCIx_HAVE_RTSCTS flag.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 4 ++--
 include/linux/serial_sci.h  | 6 ------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 6897100ed5197df3..51b436e2334c3efc 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -720,7 +720,7 @@ static void sci_init_pins(struct uart_port *port, unsigned int cflag)
 	if (!reg->size)
 		return;
 
-	if ((s->cfg->capabilities & SCIx_HAVE_RTSCTS) &&
+	if ((port->flags & UPF_HARD_FLOW) &&
 	    ((!(cflag & CRTSCTS)))) {
 		unsigned short status;
 
@@ -2247,7 +2247,7 @@ done:
 	if (reg->size) {
 		unsigned short ctrl = serial_port_in(port, SCFCR);
 
-		if (s->cfg->capabilities & SCIx_HAVE_RTSCTS) {
+		if (port->flags & UPF_HARD_FLOW) {
 			if (termios->c_cflag & CRTSCTS)
 				ctrl |= SCFCR_MCE;
 			else
diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
index 9f2bfd0557429ac3..95640ee68462190f 100644
--- a/include/linux/serial_sci.h
+++ b/include/linux/serial_sci.h
@@ -48,17 +48,11 @@ struct plat_sci_port_ops {
 };
 
 /*
- * Port-specific capabilities
- */
-#define SCIx_HAVE_RTSCTS	BIT(0)
-
-/*
  * Platform device specific platform_data struct
  */
 struct plat_sci_port {
 	unsigned int	type;			/* SCI / SCIF / IRDA / HSCIF */
 	upf_t		flags;			/* UPF_* flags */
-	unsigned long	capabilities;		/* Port features/capabilities */
 
 	unsigned int	sampling_rate;
 	unsigned int	scscr;			/* SCSCR initialization */
-- 
1.9.1

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Peter Hurley]

Hi Geert,

On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote:
> Replace the custom SCIx_HAVE_RTSCTS flag in the
> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
> the uart_port.flags and plat_sci_port.flags fields.
> Remove the now unused plat_sci_port.capabilities field.
> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.

UPF_HARD_FLOW is really for indicating the h/w supports automatic
CTS and RTS signalling; ie., RTS is automatically de-asserted when
rx fifo reaches some threshold and CTS will automatically prevent
tx fifo output.

There is no serial core flag for indicating the h/w supports (or does not)
RTS and/or CTS signalling.

Regards,
Peter Hurley

> Note that currently nothing sets the SCIx_HAVE_RTSCTS flag.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/sh-sci.c | 4 ++--
>  include/linux/serial_sci.h  | 6 ------
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 6897100ed5197df3..51b436e2334c3efc 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -720,7 +720,7 @@ static void sci_init_pins(struct uart_port *port, unsigned int cflag)
>  	if (!reg->size)
>  		return;
>  
> -	if ((s->cfg->capabilities & SCIx_HAVE_RTSCTS) &&
> +	if ((port->flags & UPF_HARD_FLOW) &&
>  	    ((!(cflag & CRTSCTS)))) {
>  		unsigned short status;
>  
> @@ -2247,7 +2247,7 @@ done:
>  	if (reg->size) {
>  		unsigned short ctrl = serial_port_in(port, SCFCR);
>  
> -		if (s->cfg->capabilities & SCIx_HAVE_RTSCTS) {
> +		if (port->flags & UPF_HARD_FLOW) {
>  			if (termios->c_cflag & CRTSCTS)
>  				ctrl |= SCFCR_MCE;
>  			else
> diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
> index 9f2bfd0557429ac3..95640ee68462190f 100644
> --- a/include/linux/serial_sci.h
> +++ b/include/linux/serial_sci.h
> @@ -48,17 +48,11 @@ struct plat_sci_port_ops {
>  };
>  
>  /*
> - * Port-specific capabilities
> - */
> -#define SCIx_HAVE_RTSCTS	BIT(0)
> -
> -/*
>   * Platform device specific platform_data struct
>   */
>  struct plat_sci_port {
>  	unsigned int	type;			/* SCI / SCIF / IRDA / HSCIF */
>  	upf_t		flags;			/* UPF_* flags */
> -	unsigned long	capabilities;		/* Port features/capabilities */
>  
>  	unsigned int	sampling_rate;
>  	unsigned int	scscr;			/* SCSCR initialization */
> 

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Geert Uytterhoeven]

Hi Peter,

On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote:
>> Replace the custom SCIx_HAVE_RTSCTS flag in the
>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
>> the uart_port.flags and plat_sci_port.flags fields.
>> Remove the now unused plat_sci_port.capabilities field.
>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.
>
> UPF_HARD_FLOW is really for indicating the h/w supports automatic
> CTS and RTS signalling; ie., RTS is automatically de-asserted when
> rx fifo reaches some threshold and CTS will automatically prevent
> tx fifo output.
>
> There is no serial core flag for indicating the h/w supports (or does not)
> RTS and/or CTS signalling.

Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins
does have support for automatic RTS/CTS signalling. The driver has (unused)
support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but
it seems to be busted when enabled.

If I understand it correctly:
  1. Automatic RTS/CTS signalling should be enabled only if the user enabled it
     through termios->c_cflag & CRTSCTS,
  2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
     the serial core, through .set_mctrl(),
  3. Regardless of the user setting CRTSCTS or not, .get_mctrl() should report
     the current status of the CTS input pin.

Are my assumptions correct?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Peter Hurley]

Hi Geert,

On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote:
> On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote:
>>> Replace the custom SCIx_HAVE_RTSCTS flag in the
>>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
>>> the uart_port.flags and plat_sci_port.flags fields.
>>> Remove the now unused plat_sci_port.capabilities field.
>>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.
>>
>> UPF_HARD_FLOW is really for indicating the h/w supports automatic
>> CTS and RTS signalling; ie., RTS is automatically de-asserted when
>> rx fifo reaches some threshold and CTS will automatically prevent
>> tx fifo output.
>>
>> There is no serial core flag for indicating the h/w supports (or does not)
>> RTS and/or CTS signalling.
> 
> Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins
> does have support for automatic RTS/CTS signalling. The driver has (unused)
> support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but
> it seems to be busted when enabled.

Ok.

So looking at the code, IIUC, SCIF does not provide a way to directly
control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio).
Is that correct?
How to support autoRTS/autoCTS depends on the answer to this question.

Is there a datasheet/trm that I can review describing register layout and
uart behavior?


> If I understand it correctly:
>   1. Automatic RTS/CTS signalling should be enabled only if the user enabled it
>      through termios->c_cflag & CRTSCTS,

yes

>   2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
>      the serial core, through .set_mctrl(),

Not quite.

_Regardless of CRTSCTS setting_, the RTS output pin should be controlled
through .set_mctrl() method. The serial core takes CRTSCTS into account on
behalf of the driver when necessary.

>   3. Regardless of the user setting CRTSCTS or not, .get_mctrl() should report
>      the current status of the CTS input pin.

yes

> Are my assumptions correct?

A couple of things.

gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not
seeing any logic in these patches to prevent that.

autoCTS without a way to read CTS input level means that although transmission
stops, the driver has no way to update port->hw_stopped so the write buffer
will keep filling up until it's full @ 4k.

autoRTS without a way to override RTS output complicates throttling.

Regards,
Peter Hurley

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Geert Uytterhoeven]

Hi Peter,

On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote:
>> On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote:
>>>> Replace the custom SCIx_HAVE_RTSCTS flag in the
>>>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
>>>> the uart_port.flags and plat_sci_port.flags fields.
>>>> Remove the now unused plat_sci_port.capabilities field.
>>>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.
>>>
>>> UPF_HARD_FLOW is really for indicating the h/w supports automatic
>>> CTS and RTS signalling; ie., RTS is automatically de-asserted when
>>> rx fifo reaches some threshold and CTS will automatically prevent
>>> tx fifo output.
>>>
>>> There is no serial core flag for indicating the h/w supports (or does not)
>>> RTS and/or CTS signalling.
>>
>> Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins
>> does have support for automatic RTS/CTS signalling. The driver has (unused)
>> support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but
>> it seems to be busted when enabled.
>
> Ok.
>
> So looking at the code, IIUC, SCIF does not provide a way to directly
> control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio).
> Is that correct?
> How to support autoRTS/autoCTS depends on the answer to this question.

Actually it does have support for controlling RTS output and reading CTS
input, but that is not yet implemented.

Magnus posted a series touching this a while ago ("serial: sh-sci: Hardware
flow control update", http://www.spinics.net/lists/linux-sh/msg38401.html),
which received some comments.

> Is there a datasheet/trm that I can review describing register layout and
> uart behavior?

I found a public one for an older SoC, see pages starting at (physical) page 849
http://documentation.renesas.com/doc/products/mpumcu/doc/superh/r01uh0048ej0200_sh7268.pdf

  - FIFO Control Register, bit MCE for automatic control
  - Serial Port Register for manual control

>>   2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
>>      the serial core, through .set_mctrl(),
>
> Not quite.
>
> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled
> through .set_mctrl() method. The serial core takes CRTSCTS into account on
> behalf of the driver when necessary.

What does this mean exactly?
AFAIU, there are three states:
  - Force RTS asserted,
  - Force RTS deasserted,
  - Use hardware-controlled automatic RTS,
while .set_mctrl() just provides the boolean TIOCM_RTS flag?

> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not
> seeing any logic in these patches to prevent that.

Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet.

I went for GPIO-based RTS/CTS first, as we now have a framework for that,
and I can use it as a known-working base.

> autoCTS without a way to read CTS input level means that although transmission
> stops, the driver has no way to update port->hw_stopped so the write buffer
> will keep filling up until it's full @ 4k.

SCIF allows to read CTS input level, regardless of automatic RTS/CTS is
enabled or not.

> autoRTS without a way to override RTS output complicates throttling.

SCIF allows to override RTS output.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Peter Hurley]

On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote:
> On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote:
>>> On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote:
>>>>> Replace the custom SCIx_HAVE_RTSCTS flag in the
>>>>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in
>>>>> the uart_port.flags and plat_sci_port.flags fields.
>>>>> Remove the now unused plat_sci_port.capabilities field.
>>>>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags.
>>>>
>>>> UPF_HARD_FLOW is really for indicating the h/w supports automatic
>>>> CTS and RTS signalling; ie., RTS is automatically de-asserted when
>>>> rx fifo reaches some threshold and CTS will automatically prevent
>>>> tx fifo output.
>>>>
>>>> There is no serial core flag for indicating the h/w supports (or does not)
>>>> RTS and/or CTS signalling.
>>>
>>> Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins
>>> does have support for automatic RTS/CTS signalling. The driver has (unused)
>>> support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but
>>> it seems to be busted when enabled.
>>
>> Ok.
>>
>> So looking at the code, IIUC, SCIF does not provide a way to directly
>> control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio).
>> Is that correct?
>> How to support autoRTS/autoCTS depends on the answer to this question.
> 
> Actually it does have support for controlling RTS output and reading CTS
> input, but that is not yet implemented.
> 
> Magnus posted a series touching this a while ago ("serial: sh-sci: Hardware
> flow control update", http://www.spinics.net/lists/linux-sh/msg38401.html),
> which received some comments.
> 
>> Is there a datasheet/trm that I can review describing register layout and
>> uart behavior?
> 
> I found a public one for an older SoC, see pages starting at (physical) page 849
> http://documentation.renesas.com/doc/products/mpumcu/doc/superh/r01uh0048ej0200_sh7268.pdf

Thanks.

> 
>   - FIFO Control Register, bit MCE for automatic control
>   - Serial Port Register for manual control

Actually the FIFO Control Register doesn't say anything regarding automatic
control, but 16.4.2 Operation in Asynchronous Mode, Section (3) Transmitting and
Receiving Data gives examples of operation when "modem control is enabled".

Those examples show autoRTS/autoCTS behavior.


[discussion of set_mctrl() and autoRTS moved to EOM]


 
>> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not
>> seeing any logic in these patches to prevent that.
> 
> Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet.

Anything that enables the gpios which will be in DT and beyond our control
needs to exclude autoRTS/autoCTS within the patch that provides the
DT functionality.


> I went for GPIO-based RTS/CTS first, as we now have a framework for that,
> and I can use it as a known-working base.

Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to
contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs.


>> autoCTS without a way to read CTS input level means that although transmission
>> stops, the driver has no way to update port->hw_stopped so the write buffer
>> will keep filling up until it's full @ 4k.
> 
> SCIF allows to read CTS input level, regardless of automatic RTS/CTS is
> enabled or not.

I see that but crucial support for CRTSCTS is missing, AFAICT; namely,
a modem status interrupt.

There's no way to determine when the remote re-enables CTS without polling,
and the serial core provides no mechanism for the driver to poll CTS.

So again, the driver has no way to trigger stopping/restarting tx when CTS
changes without autoCTS. (A modem status interrupt for dCTS should call
uart_handle_cts_change() to perform this operation).


>> autoRTS without a way to override RTS output complicates throttling.
> 
> SCIF allows to override RTS output.

Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states
"When the RTS pin is actually used as a port outputting
the RTSDT bit value, the MCE bit in SCFCR should be
cleared to 0."

IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value
outputting at the pin.



>>>   2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
>>>      the serial core, through .set_mctrl(),
>>
>> Not quite.
>>
>> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled
>> through .set_mctrl() method. The serial core takes CRTSCTS into account on
>> behalf of the driver when necessary.
> 
> What does this mean exactly?
> AFAIU, there are three states:
>   - Force RTS asserted,
>   - Force RTS deasserted,
>   - Use hardware-controlled automatic RTS,
> while .set_mctrl() just provides the boolean TIOCM_RTS flag?

Since there is no userspace knob for autoRTS, drivers that enable autoRTS
with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause
RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise)
for set_mctrl(TIOCM_RTS).

Regards,
Peter Hurley

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Geert Uytterhoeven]

Hi Peter,

On Thu, Mar 24, 2016 at 1:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote:
>> On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not
>>> seeing any logic in these patches to prevent that.
>>
>> Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet.
>
> Anything that enables the gpios which will be in DT and beyond our control
> needs to exclude autoRTS/autoCTS within the patch that provides the
> DT functionality.
>
>> I went for GPIO-based RTS/CTS first, as we now have a framework for that,
>> and I can use it as a known-working base.
>
> Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to
> contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs.

AutoRTS/autoCTS cannot be enabled yet from DT.
Hence on DT platforms, the driver uses either gpio-based RTS/CTS, or no RTS/CTS
at all.

>>> autoCTS without a way to read CTS input level means that although transmission
>>> stops, the driver has no way to update port->hw_stopped so the write buffer
>>> will keep filling up until it's full @ 4k.
>>
>> SCIF allows to read CTS input level, regardless of automatic RTS/CTS is
>> enabled or not.
>
> I see that but crucial support for CRTSCTS is missing, AFAICT; namely,
> a modem status interrupt.

There's indeed no interrupt support for that, unless you use a GPIO.

> There's no way to determine when the remote re-enables CTS without polling,
> and the serial core provides no mechanism for the driver to poll CTS.

OK, so it's the responsibility of the driver to poll CTS, if there's no CTS
interrupt support. Even when autoCTS is enabled.

> So again, the driver has no way to trigger stopping/restarting tx when CTS
> changes without autoCTS. (A modem status interrupt for dCTS should call
> uart_handle_cts_change() to perform this operation).

But if the hardware has autoCTS, we should use it, right?
Hence "... without autoCTS" is never true if the hardware has autoCTS?

BTW, this means that drivers using mctrl_gpio_init_noauto(), but not
implementing autoCTS(), and not polling CTS are broken?

drivers/tty/serial/clps711x.c looks fishy: it doesn't poll CTS, but always sets
TIOCM_CTS, probably to work around this?

>>> autoRTS without a way to override RTS output complicates throttling.
>>
>> SCIF allows to override RTS output.
>
> Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states
> "When the RTS pin is actually used as a port outputting
> the RTSDT bit value, the MCE bit in SCFCR should be
> cleared to 0."
>
> IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value
> outputting at the pin.

That's also my understanding: to manually control the RTS pin, you have to
disable automatic flow control (or use pinmux to change the pin to a GPIO).

[*]

>>>>   2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
>>>>      the serial core, through .set_mctrl(),
>>>
>>> Not quite.
>>>
>>> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled
>>> through .set_mctrl() method. The serial core takes CRTSCTS into account on
>>> behalf of the driver when necessary.
>>
>> What does this mean exactly?
>> AFAIU, there are three states:
>>   - Force RTS asserted,

(let's call this state 1)

>>   - Force RTS deasserted,

(state 2)

>>   - Use hardware-controlled automatic RTS,

(state 3)

>> while .set_mctrl() just provides the boolean TIOCM_RTS flag?
>
> Since there is no userspace knob for autoRTS, drivers that enable autoRTS
> with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause
> RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise)
> for set_mctrl(TIOCM_RTS).

OK, makes sense. That's also what I was guessing. So when autoRTS is enabled,
we have either state 2 or state 3.

Now, why would you want to override RTS output, and is [*] above an issue?
Is it because you want autoCTS, but not autoRTS?

Thanks for your answers, and sorry for still having that many questions...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 5/5] serial: sh-sci: Replace SCIx_HAVE_RTSCTS by standard UPF_HARD_FLOW

[Peter Hurley]

Hi Geert,

On 03/24/2016 06:21 AM, Geert Uytterhoeven wrote:
> On Thu, Mar 24, 2016 at 1:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote:
>>> On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not
>>>> seeing any logic in these patches to prevent that.
>>>
>>> Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet.
>>
>> Anything that enables the gpios which will be in DT and beyond our control
>> needs to exclude autoRTS/autoCTS within the patch that provides the
>> DT functionality.
>>
>>> I went for GPIO-based RTS/CTS first, as we now have a framework for that,
>>> and I can use it as a known-working base.
>>
>> Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to
>> contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs.
> 
> AutoRTS/autoCTS cannot be enabled yet from DT.
> Hence on DT platforms, the driver uses either gpio-based RTS/CTS, or no RTS/CTS
> at all.

I do understand that the situation is mutually exclusive currently,
and even after the gpio patches. I'm probably just being overly-cautious here.

There have been a couple of situations where a DT configuration was invalid
and some ugly hoops were required to prevent it in the driver; eg., famously
the OMAP dma fubar.

I guess I'd be okay with skipping it as long as I knew someone was going to
make sure it got addressed soonish.


>>>> autoCTS without a way to read CTS input level means that although transmission
>>>> stops, the driver has no way to update port->hw_stopped so the write buffer
>>>> will keep filling up until it's full @ 4k.
>>>
>>> SCIF allows to read CTS input level, regardless of automatic RTS/CTS is
>>> enabled or not.
>>
>> I see that but crucial support for CRTSCTS is missing, AFAICT; namely,
>> a modem status interrupt.
> 
> There's indeed no interrupt support for that, unless you use a GPIO.
> 
>> There's no way to determine when the remote re-enables CTS without polling,
>> and the serial core provides no mechanism for the driver to poll CTS.
> 
> OK, so it's the responsibility of the driver to poll CTS, if there's no CTS
> interrupt support.

Sorry, no, I didn't mean the driver should try to implement some polling
scheme on its own.

> Even when autoCTS is enabled.

For autoCTS when the driver/hardware does not have CTS interrupt support,
set UPSTAT_AUTOCTS in port->status. This will prevent the serial core from
stopping the tty if CTS is not active (since the driver/hardware cannot
restart the tty when CTS is set active).

For !autoCTS when the driver/hardware does not have CTS interrupt support,
get_mctrl() must always report CTS active. [Another potential solution is
to disable CRTSCTS in the driver's set_termios method: this would be
preferable but I'm not sure all userspace will be ok with this.]


>> So again, the driver has no way to trigger stopping/restarting tx when CTS
>> changes without autoCTS. (A modem status interrupt for dCTS should call
>> uart_handle_cts_change() to perform this operation).
> 
> But if the hardware has autoCTS, we should use it, right?

Yes, but I consider that a refinement over basic CTS/RTS handling, and
so not an actual driver requirement.

> Hence "... without autoCTS" is never true if the hardware has autoCTS?

Well, a driver can only support s/w assisted h/w flow control*, even though
the h/w is capable of autoCTS.

* enabling/disabling tx on dCTS interrupt


> BTW, this means that drivers using mctrl_gpio_init_noauto(), but not
> implementing autoCTS(), and not polling CTS are broken?
> 
> drivers/tty/serial/clps711x.c looks fishy:

This looks broken.

If CRTSCTS is set and CTS is pinned to a gpio, tx will continue even when
the CTS gpio is inactive.

Also, if the CTS gpio is inactive when the tty is opened or the termios is
changed, tx will be stuck off.


> it doesn't poll CTS, but always sets TIOCM_CTS, probably to work around this?

This driver is not always setting TIOCM_CTS; that's just the default if CTS
is not pinned to a gpio.


>>>> autoRTS without a way to override RTS output complicates throttling.
>>>
>>> SCIF allows to override RTS output.
>>
>> Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states
>> "When the RTS pin is actually used as a port outputting
>> the RTSDT bit value, the MCE bit in SCFCR should be
>> cleared to 0."
>>
>> IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value
>> outputting at the pin.
> 
> That's also my understanding: to manually control the RTS pin, you have to
> disable automatic flow control (or use pinmux to change the pin to a GPIO).
> 
> [*]
> 
>>>>>   2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by
>>>>>      the serial core, through .set_mctrl(),
>>>>
>>>> Not quite.
>>>>
>>>> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled
>>>> through .set_mctrl() method. The serial core takes CRTSCTS into account on
>>>> behalf of the driver when necessary.
>>>
>>> What does this mean exactly?
>>> AFAIU, there are three states:
>>>   - Force RTS asserted,
> 
> (let's call this state 1)
> 
>>>   - Force RTS deasserted,
> 
> (state 2)
> 
>>>   - Use hardware-controlled automatic RTS,
> 
> (state 3)
> 
>>> while .set_mctrl() just provides the boolean TIOCM_RTS flag?
>>
>> Since there is no userspace knob for autoRTS, drivers that enable autoRTS
>> with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause
>> RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise)
>> for set_mctrl(TIOCM_RTS).
> 
> OK, makes sense. That's also what I was guessing. So when autoRTS is enabled,
> we have either state 2 or state 3.

Yes.


> Now, why would you want to override RTS output,

A couple of reasons:

1. userspace terminal control will expect to be able to use
   TIOCMSET/TIOCMBIC/TIOCMBIS to stop/resume input by dropping RTS when
   using CRTSCTS.
2. userspace drivers for uart-interfaced devices use TIOCMSET on RTS for things
   like device wakeup.
3. in-tree drivers use tiocmset() directly for the same thing.


> and is [*] above an issue?

Yes. The driver must disable autoRTS in set_mctrl() if TIOCM_RTS is clear and
restore the autoRTS/RTS state if TIOCM_RTS is set.
The omap8250 driver does this (omap8250_set_mctrl()).

IMPORTANT NOTE: do _not_ use UPSTAT_AUTORTS to track your autoRTS state, like
the omap8250 driver does.

If you do, you need to provide throttle/unthrottle methods to stop and resume
remote input; however, I don't think the methods currently used by these
drivers is appropriate or safe (by letting the rx fifo fill up).

> Is it because you want autoCTS, but not autoRTS?

No, but that does happen.


> Thanks for your answers, and sorry for still having that many questions...

Not your fault. I should really document this.

Regards,
Peter Hurley