diff mbox

[v2] serial: uart: add hw flow control support configuration

Message ID 1398971093-17164-1-git-send-email-m-karicheri2@ti.com
State Accepted
Commit 06aa82e498c144c7784a6f3d3b55458b272d6146
Headers show

Commit Message

Murali Karicheri May 1, 2014, 7:04 p.m. UTC
8250 uart driver currently supports only software assisted hw flow
control. The software assisted hw flow control maintains a hw_stopped
flag in the tty structure to stop and start transmission and use modem
status interrupt for the event to drive the handshake signals. This is
not needed if hw has flow control capabilities. This patch adds a
DT attribute for enabling hw flow control for a uart port. Also skip
stop and start if this flag is present in flag field of the port
structure.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

CC: Rob Herring <robh+dt@kernel.org>
CC: Pawel Moll <pawel.moll@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
CC: Kumar Gala <galak@codeaurora.org>
CC: Randy Dunlap <rdunlap@infradead.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Jiri Slaby <jslaby@suse.cz>
CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 .../devicetree/bindings/serial/of-serial.txt       |    1 +
 drivers/tty/serial/8250/8250_core.c                |    6 ++++--
 drivers/tty/serial/of_serial.c                     |    4 ++++
 drivers/tty/serial/serial_core.c                   |   12 +++++++++---
 4 files changed, 18 insertions(+), 5 deletions(-)

Comments

Murali Karicheri May 9, 2014, 3:30 p.m. UTC | #1
>-----Original Message-----
>From: Karicheri, Muralidharan
>Sent: Thursday, May 01, 2014 3:05 PM
>To: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
>linux-serial@vger.kernel.org
>Cc: Karicheri, Muralidharan; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar
>Gala; Randy Dunlap; Greg Kroah-Hartman; Jiri Slaby; Shilimkar, Santosh
>Subject: [PATCH v2] serial: uart: add hw flow control support configuration
>
>8250 uart driver currently supports only software assisted hw flow control. The software
>assisted hw flow control maintains a hw_stopped flag in the tty structure to stop and start
>transmission and use modem status interrupt for the event to drive the handshake signals.
>This is not needed if hw has flow control capabilities. This patch adds a DT attribute for
>enabling hw flow control for a uart port. Also skip stop and start if this flag is present in flag
>field of the port structure.
>
>Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>
>CC: Rob Herring <robh+dt@kernel.org>
>CC: Pawel Moll <pawel.moll@arm.com>
>CC: Mark Rutland <mark.rutland@arm.com>
>CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
>CC: Kumar Gala <galak@codeaurora.org>
>CC: Randy Dunlap <rdunlap@infradead.org>
>CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>CC: Jiri Slaby <jslaby@suse.cz>
>CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
>---
> .../devicetree/bindings/serial/of-serial.txt       |    1 +
> drivers/tty/serial/8250/8250_core.c                |    6 ++++--
> drivers/tty/serial/of_serial.c                     |    4 ++++
> drivers/tty/serial/serial_core.c                   |   12 +++++++++---
> 4 files changed, 18 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt
>b/Documentation/devicetree/bindings/serial/of-serial.txt
>index 1928a3e..7705477 100644
>--- a/Documentation/devicetree/bindings/serial/of-serial.txt
>+++ b/Documentation/devicetree/bindings/serial/of-serial.txt
>@@ -37,6 +37,7 @@ Optional properties:
> - auto-flow-control: one way to enable automatic flow control support. The
>   driver is allowed to detect support for the capability even without this
>   property.
>+- has-hw-flow-control: the hardware has flow control capability.
>
> Example:
>
>diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>index 0e1bf88..b69aff2 100644
>--- a/drivers/tty/serial/8250/8250_core.c
>+++ b/drivers/tty/serial/8250/8250_core.c
>@@ -2338,9 +2338,11 @@ serial8250_do_set_termios(struct uart_port *port, struct
>ktermios *termios,
> 	 * the trigger, or the MCR RTS bit is cleared.  In the case where
> 	 * the remote UART is not using CTS auto flow control, we must
> 	 * have sufficient FIFO entries for the latency of the remote
>-	 * UART to respond.  IOW, at least 32 bytes of FIFO.
>+	 * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
>+	 * AFE if hw flow control is supported
> 	 */
>-	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
>+	if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32)) ||
>+	    (port->flags & UPF_HARD_FLOW)) {
> 		up->mcr &= ~UART_MCR_AFE;
> 		if (termios->c_cflag & CRTSCTS)
> 			up->mcr |= UART_MCR_AFE;
>diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c index
>9924660..77ec6a1 100644
>--- a/drivers/tty/serial/of_serial.c
>+++ b/drivers/tty/serial/of_serial.c
>@@ -182,6 +182,10 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
> 					  "auto-flow-control"))
> 			port8250.capabilities |= UART_CAP_AFE;
>
>+		if (of_property_read_bool(ofdev->dev.of_node,
>+					  "has-hw-flow-control"))
>+			port8250.port.flags |= UPF_HARD_FLOW;
>+
> 		ret = serial8250_register_8250_port(&port8250);
> 		break;
> 	}
>diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>index b68550d..851707a 100644
>--- a/drivers/tty/serial/serial_core.c
>+++ b/drivers/tty/serial/serial_core.c
>@@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct
>uart_state *state,
> 			if (tty->termios.c_cflag & CBAUD)
> 				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
> 		}
>-
>-		if (tty_port_cts_enabled(port)) {
>+		/*
>+		 * if hw support flow control without software intervention,
>+		 * then skip the below check
>+		 */
>+		if (tty_port_cts_enabled(port) &&
>+		    !(uport->flags & UPF_HARD_FLOW)) {
> 			spin_lock_irq(&uport->lock);
> 			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
> 				tty->hw_stopped = 1;
>@@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned
>int status)
>
> 	uport->icount.cts++;
>
>-	if (tty_port_cts_enabled(port)) {
>+	/* skip below code if the hw flow control is supported */
>+	if (tty_port_cts_enabled(port) &&
>+	    !(uport->flags & UPF_HARD_FLOW)) {
> 		if (tty->hw_stopped) {
> 			if (status) {
> 				tty->hw_stopped = 0;
>--
>1.7.9.5

Any idea who is going to pick this patch for merge?

Murali
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
dann frazier May 28, 2014, 8:04 p.m. UTC | #2
On Thu, May 1, 2014 at 1:04 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> 8250 uart driver currently supports only software assisted hw flow
> control. The software assisted hw flow control maintains a hw_stopped
> flag in the tty structure to stop and start transmission and use modem
> status interrupt for the event to drive the handshake signals. This is
> not needed if hw has flow control capabilities. This patch adds a
> DT attribute for enabling hw flow control for a uart port. Also skip
> stop and start if this flag is present in flag field of the port
> structure.

ubuntu@hwflow:~$ sudo stty -a --file /dev/ttyS0 |tr ' ' '\n' | grep crtscts
crtscts
ubuntu@hwflow:~$ ls /proc/device-tree/soc/serial@1c021000/has-hw-flow-control
/proc/device-tree/soc/serial@1c021000/has-hw-flow-control
ubuntu@hwflow:~$ python
Python 2.7.6 (default, Mar 22 2014, 22:58:30)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> UPF_HARDWARE_FLOW = 1 << 21
>>> if 0xB9200000 & UPF_HARDWARE_FLOW:
...     print "OK"
...
OK

Hope that's a reasonable test case. Test fails when booted w/o
has-hw-flow-control attribute.

Tested-by: dann frazier <dann.frazier@canonical.com>

>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Pawel Moll <pawel.moll@arm.com>
> CC: Mark Rutland <mark.rutland@arm.com>
> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
> CC: Kumar Gala <galak@codeaurora.org>
> CC: Randy Dunlap <rdunlap@infradead.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Jiri Slaby <jslaby@suse.cz>
> CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  .../devicetree/bindings/serial/of-serial.txt       |    1 +
>  drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>  drivers/tty/serial/of_serial.c                     |    4 ++++
>  drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>  4 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
> index 1928a3e..7705477 100644
> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
> @@ -37,6 +37,7 @@ Optional properties:
>  - auto-flow-control: one way to enable automatic flow control support. The
>    driver is allowed to detect support for the capability even without this
>    property.
> +- has-hw-flow-control: the hardware has flow control capability.
>
>  Example:
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 0e1bf88..b69aff2 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -2338,9 +2338,11 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>          * the trigger, or the MCR RTS bit is cleared.  In the case where
>          * the remote UART is not using CTS auto flow control, we must
>          * have sufficient FIFO entries for the latency of the remote
> -        * UART to respond.  IOW, at least 32 bytes of FIFO.
> +        * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
> +        * AFE if hw flow control is supported
>          */
> -       if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
> +       if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32)) ||
> +           (port->flags & UPF_HARD_FLOW)) {
>                 up->mcr &= ~UART_MCR_AFE;
>                 if (termios->c_cflag & CRTSCTS)
>                         up->mcr |= UART_MCR_AFE;
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index 9924660..77ec6a1 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -182,6 +182,10 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>                                           "auto-flow-control"))
>                         port8250.capabilities |= UART_CAP_AFE;
>
> +               if (of_property_read_bool(ofdev->dev.of_node,
> +                                         "has-hw-flow-control"))
> +                       port8250.port.flags |= UPF_HARD_FLOW;
> +
>                 ret = serial8250_register_8250_port(&port8250);
>                 break;
>         }
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index b68550d..851707a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>                         if (tty->termios.c_cflag & CBAUD)
>                                 uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>                 }
> -
> -               if (tty_port_cts_enabled(port)) {
> +               /*
> +                * if hw support flow control without software intervention,
> +                * then skip the below check
> +                */
> +               if (tty_port_cts_enabled(port) &&
> +                   !(uport->flags & UPF_HARD_FLOW)) {
>                         spin_lock_irq(&uport->lock);
>                         if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
>                                 tty->hw_stopped = 1;
> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>
>         uport->icount.cts++;
>
> -       if (tty_port_cts_enabled(port)) {
> +       /* skip below code if the hw flow control is supported */
> +       if (tty_port_cts_enabled(port) &&
> +           !(uport->flags & UPF_HARD_FLOW)) {
>                 if (tty->hw_stopped) {
>                         if (status) {
>                                 tty->hw_stopped = 0;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Murali Karicheri June 11, 2014, 8:53 p.m. UTC | #3
On 5/28/2014 4:04 PM, Dann Frazier wrote:
> On Thu, May 1, 2014 at 1:04 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
>> 8250 uart driver currently supports only software assisted hw flow
>> control. The software assisted hw flow control maintains a hw_stopped
>> flag in the tty structure to stop and start transmission and use modem
>> status interrupt for the event to drive the handshake signals. This is
>> not needed if hw has flow control capabilities. This patch adds a
>> DT attribute for enabling hw flow control for a uart port. Also skip
>> stop and start if this flag is present in flag field of the port
>> structure.
> ubuntu@hwflow:~$ sudo stty -a --file /dev/ttyS0 |tr ' ' '\n' | grep crtscts
> crtscts
> ubuntu@hwflow:~$ ls /proc/device-tree/soc/serial@1c021000/has-hw-flow-control
> /proc/device-tree/soc/serial@1c021000/has-hw-flow-control
> ubuntu@hwflow:~$ python
> Python 2.7.6 (default, Mar 22 2014, 22:58:30)
> [GCC 4.8.2] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
>>>> UPF_HARDWARE_FLOW = 1 << 21
>>>> if 0xB9200000 & UPF_HARDWARE_FLOW:
> ...     print "OK"
> ...
> OK
>
> Hope that's a reasonable test case. Test fails when booted w/o
> has-hw-flow-control attribute.
>
> Tested-by: dann frazier <dann.frazier@canonical.com>
What is the verdict? pass/fail? Ok/Not OK to merge?

Murali
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>
>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Pawel Moll <pawel.moll@arm.com>
>> CC: Mark Rutland <mark.rutland@arm.com>
>> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> CC: Kumar Gala <galak@codeaurora.org>
>> CC: Randy Dunlap <rdunlap@infradead.org>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> CC: Jiri Slaby <jslaby@suse.cz>
>> CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
>> index 1928a3e..7705477 100644
>> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
>> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
>> @@ -37,6 +37,7 @@ Optional properties:
>>   - auto-flow-control: one way to enable automatic flow control support. The
>>     driver is allowed to detect support for the capability even without this
>>     property.
>> +- has-hw-flow-control: the hardware has flow control capability.
>>
>>   Example:
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 0e1bf88..b69aff2 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -2338,9 +2338,11 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>>           * the trigger, or the MCR RTS bit is cleared.  In the case where
>>           * the remote UART is not using CTS auto flow control, we must
>>           * have sufficient FIFO entries for the latency of the remote
>> -        * UART to respond.  IOW, at least 32 bytes of FIFO.
>> +        * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
>> +        * AFE if hw flow control is supported
>>           */
>> -       if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
>> +       if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32)) ||
>> +           (port->flags & UPF_HARD_FLOW)) {
>>                  up->mcr &= ~UART_MCR_AFE;
>>                  if (termios->c_cflag & CRTSCTS)
>>                          up->mcr |= UART_MCR_AFE;
>> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
>> index 9924660..77ec6a1 100644
>> --- a/drivers/tty/serial/of_serial.c
>> +++ b/drivers/tty/serial/of_serial.c
>> @@ -182,6 +182,10 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
>>                                            "auto-flow-control"))
>>                          port8250.capabilities |= UART_CAP_AFE;
>>
>> +               if (of_property_read_bool(ofdev->dev.of_node,
>> +                                         "has-hw-flow-control"))
>> +                       port8250.port.flags |= UPF_HARD_FLOW;
>> +
>>                  ret = serial8250_register_8250_port(&port8250);
>>                  break;
>>          }
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index b68550d..851707a 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>                          if (tty->termios.c_cflag & CBAUD)
>>                                  uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>                  }
>> -
>> -               if (tty_port_cts_enabled(port)) {
>> +               /*
>> +                * if hw support flow control without software intervention,
>> +                * then skip the below check
>> +                */
>> +               if (tty_port_cts_enabled(port) &&
>> +                   !(uport->flags & UPF_HARD_FLOW)) {
>>                          spin_lock_irq(&uport->lock);
>>                          if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
>>                                  tty->hw_stopped = 1;
>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>
>>          uport->icount.cts++;
>>
>> -       if (tty_port_cts_enabled(port)) {
>> +       /* skip below code if the hw flow control is supported */
>> +       if (tty_port_cts_enabled(port) &&
>> +           !(uport->flags & UPF_HARD_FLOW)) {
>>                  if (tty->hw_stopped) {
>>                          if (status) {
>>                                  tty->hw_stopped = 0;
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
dann frazier June 11, 2014, 9:05 p.m. UTC | #4
On Wed, Jun 11, 2014 at 2:53 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> On 5/28/2014 4:04 PM, Dann Frazier wrote:
>>
>> On Thu, May 1, 2014 at 1:04 PM, Murali Karicheri <m-karicheri2@ti.com>
>> wrote:
>>>
>>> 8250 uart driver currently supports only software assisted hw flow
>>> control. The software assisted hw flow control maintains a hw_stopped
>>> flag in the tty structure to stop and start transmission and use modem
>>> status interrupt for the event to drive the handshake signals. This is
>>> not needed if hw has flow control capabilities. This patch adds a
>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>> stop and start if this flag is present in flag field of the port
>>> structure.
>>
>> ubuntu@hwflow:~$ sudo stty -a --file /dev/ttyS0 |tr ' ' '\n' | grep
>> crtscts
>> crtscts
>> ubuntu@hwflow:~$ ls
>> /proc/device-tree/soc/serial@1c021000/has-hw-flow-control
>> /proc/device-tree/soc/serial@1c021000/has-hw-flow-control
>> ubuntu@hwflow:~$ python
>> Python 2.7.6 (default, Mar 22 2014, 22:58:30)
>> [GCC 4.8.2] on linux2
>> Type "help", "copyright", "credits" or "license" for more information.
>>>>>
>>>>> UPF_HARDWARE_FLOW = 1 << 21
>>>>> if 0xB9200000 & UPF_HARDWARE_FLOW:
>>
>> ...     print "OK"
>> ...
>> OK
>>
>> Hope that's a reasonable test case. Test fails when booted w/o
>> has-hw-flow-control attribute.
>>
>> Tested-by: dann frazier <dann.frazier@canonical.com>
>
> What is the verdict? pass/fail? Ok/Not OK to merge?

Murali,

 It is working for me, and appears to already be Linus' tree.

  -dann

> Murali
>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>>
>>> CC: Rob Herring <robh+dt@kernel.org>
>>> CC: Pawel Moll <pawel.moll@arm.com>
>>> CC: Mark Rutland <mark.rutland@arm.com>
>>> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
>>> CC: Kumar Gala <galak@codeaurora.org>
>>> CC: Randy Dunlap <rdunlap@infradead.org>
>>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> CC: Jiri Slaby <jslaby@suse.cz>
>>> CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> ---
>>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt
>>> b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> index 1928a3e..7705477 100644
>>> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
>>> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> @@ -37,6 +37,7 @@ Optional properties:
>>>   - auto-flow-control: one way to enable automatic flow control support.
>>> The
>>>     driver is allowed to detect support for the capability even without
>>> this
>>>     property.
>>> +- has-hw-flow-control: the hardware has flow control capability.
>>>
>>>   Example:
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_core.c
>>> b/drivers/tty/serial/8250/8250_core.c
>>> index 0e1bf88..b69aff2 100644
>>> --- a/drivers/tty/serial/8250/8250_core.c
>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>> @@ -2338,9 +2338,11 @@ serial8250_do_set_termios(struct uart_port *port,
>>> struct ktermios *termios,
>>>           * the trigger, or the MCR RTS bit is cleared.  In the case
>>> where
>>>           * the remote UART is not using CTS auto flow control, we must
>>>           * have sufficient FIFO entries for the latency of the remote
>>> -        * UART to respond.  IOW, at least 32 bytes of FIFO.
>>> +        * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
>>> +        * AFE if hw flow control is supported
>>>           */
>>> -       if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
>>> +       if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32))
>>> ||
>>> +           (port->flags & UPF_HARD_FLOW)) {
>>>                  up->mcr &= ~UART_MCR_AFE;
>>>                  if (termios->c_cflag & CRTSCTS)
>>>                          up->mcr |= UART_MCR_AFE;
>>> diff --git a/drivers/tty/serial/of_serial.c
>>> b/drivers/tty/serial/of_serial.c
>>> index 9924660..77ec6a1 100644
>>> --- a/drivers/tty/serial/of_serial.c
>>> +++ b/drivers/tty/serial/of_serial.c
>>> @@ -182,6 +182,10 @@ static int of_platform_serial_probe(struct
>>> platform_device *ofdev)
>>>                                            "auto-flow-control"))
>>>                          port8250.capabilities |= UART_CAP_AFE;
>>>
>>> +               if (of_property_read_bool(ofdev->dev.of_node,
>>> +                                         "has-hw-flow-control"))
>>> +                       port8250.port.flags |= UPF_HARD_FLOW;
>>> +
>>>                  ret = serial8250_register_8250_port(&port8250);
>>>                  break;
>>>          }
>>> diff --git a/drivers/tty/serial/serial_core.c
>>> b/drivers/tty/serial/serial_core.c
>>> index b68550d..851707a 100644
>>> --- a/drivers/tty/serial/serial_core.c
>>> +++ b/drivers/tty/serial/serial_core.c
>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty,
>>> struct uart_state *state,
>>>                          if (tty->termios.c_cflag & CBAUD)
>>>                                  uart_set_mctrl(uport, TIOCM_RTS |
>>> TIOCM_DTR);
>>>                  }
>>> -
>>> -               if (tty_port_cts_enabled(port)) {
>>> +               /*
>>> +                * if hw support flow control without software
>>> intervention,
>>> +                * then skip the below check
>>> +                */
>>> +               if (tty_port_cts_enabled(port) &&
>>> +                   !(uport->flags & UPF_HARD_FLOW)) {
>>>                          spin_lock_irq(&uport->lock);
>>>                          if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
>>>                                  tty->hw_stopped = 1;
>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port
>>> *uport, unsigned int status)
>>>
>>>          uport->icount.cts++;
>>>
>>> -       if (tty_port_cts_enabled(port)) {
>>> +       /* skip below code if the hw flow control is supported */
>>> +       if (tty_port_cts_enabled(port) &&
>>> +           !(uport->flags & UPF_HARD_FLOW)) {
>>>                  if (tty->hw_stopped) {
>>>                          if (status) {
>>>                                  tty->hw_stopped = 0;
>>> --
>>> 1.7.9.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>>
>
--
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
Peter Hurley Aug. 7, 2014, 3:29 p.m. UTC | #5
On 05/01/2014 03:04 PM, Murali Karicheri wrote:
> 8250 uart driver currently supports only software assisted hw flow
> control. The software assisted hw flow control maintains a hw_stopped
> flag in the tty structure to stop and start transmission and use modem
> status interrupt for the event to drive the handshake signals. This is
> not needed if hw has flow control capabilities. This patch adds a
> DT attribute for enabling hw flow control for a uart port. Also skip
> stop and start if this flag is present in flag field of the port
> structure.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> 
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Pawel Moll <pawel.moll@arm.com>
> CC: Mark Rutland <mark.rutland@arm.com>
> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
> CC: Kumar Gala <galak@codeaurora.org>
> CC: Randy Dunlap <rdunlap@infradead.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Jiri Slaby <jslaby@suse.cz>
> CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  .../devicetree/bindings/serial/of-serial.txt       |    1 +
>  drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>  drivers/tty/serial/of_serial.c                     |    4 ++++
>  drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>  4 files changed, 18 insertions(+), 5 deletions(-)
> 

[...]

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index b68550d..851707a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>  			if (tty->termios.c_cflag & CBAUD)
>  				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>  		}
> -
> -		if (tty_port_cts_enabled(port)) {
> +		/*
> +		 * if hw support flow control without software intervention,
> +		 * then skip the below check
> +		 */
> +		if (tty_port_cts_enabled(port) &&
> +		    !(uport->flags & UPF_HARD_FLOW)) {
>  			spin_lock_irq(&uport->lock);
>  			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
>  				tty->hw_stopped = 1;
> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>  
>  	uport->icount.cts++;
>  
> -	if (tty_port_cts_enabled(port)) {
> +	/* skip below code if the hw flow control is supported */
> +	if (tty_port_cts_enabled(port) &&
> +	    !(uport->flags & UPF_HARD_FLOW)) {

Why is a modem status interrupt being generated for DCTS
if autoflow control is enabled?

This should be:

	WARN_ON_ONCE(uport->flags & UPF_HARD_FLOW);

to indicate a mis-configured driver/device.

>  		if (tty->hw_stopped) {
>  			if (status) {
>  				tty->hw_stopped = 0;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri Aug. 7, 2014, 4:16 p.m. UTC | #6
On 08/07/2014 11:29 AM, Peter Hurley wrote:
> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>> 8250 uart driver currently supports only software assisted hw flow
>> control. The software assisted hw flow control maintains a hw_stopped
>> flag in the tty structure to stop and start transmission and use modem
>> status interrupt for the event to drive the handshake signals. This is
>> not needed if hw has flow control capabilities. This patch adds a
>> DT attribute for enabling hw flow control for a uart port. Also skip
>> stop and start if this flag is present in flag field of the port
>> structure.
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>
>> CC: Rob Herring<robh+dt@kernel.org>
>> CC: Pawel Moll<pawel.moll@arm.com>
>> CC: Mark Rutland<mark.rutland@arm.com>
>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>> CC: Kumar Gala<galak@codeaurora.org>
>> CC: Randy Dunlap<rdunlap@infradead.org>
>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>> CC: Jiri Slaby<jslaby@suse.cz>
>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> ---
>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>
> [...]
>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index b68550d..851707a 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>   			if (tty->termios.c_cflag&  CBAUD)
>>   				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>   		}
>> -
>> -		if (tty_port_cts_enabled(port)) {
>> +		/*
>> +		 * if hw support flow control without software intervention,
>> +		 * then skip the below check
>> +		 */
>> +		if (tty_port_cts_enabled(port)&&
>> +		    !(uport->flags&  UPF_HARD_FLOW)) {
>>   			spin_lock_irq(&uport->lock);
>>   			if (!(uport->ops->get_mctrl(uport)&  TIOCM_CTS))
>>   				tty->hw_stopped = 1;
>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>
>>   	uport->icount.cts++;
>>
>> -	if (tty_port_cts_enabled(port)) {
>> +	/* skip below code if the hw flow control is supported */
>> +	if (tty_port_cts_enabled(port)&&
>> +	    !(uport->flags&  UPF_HARD_FLOW)) {
> Why is a modem status interrupt being generated for DCTS
> if autoflow control is enabled?
>
> This should be:
>
> 	WARN_ON_ONCE(uport->flags&  UPF_HARD_FLOW);
>
> to indicate a mis-configured driver/device.
This patch is already merged to the upstream branch and if you see any 
issue, please
post a patch for review.

Murali
>
>>   		if (tty->hw_stopped) {
>>   			if (status) {
>>   				tty->hw_stopped = 0;
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman Aug. 7, 2014, 5:12 p.m. UTC | #7
On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
> On 08/07/2014 11:29 AM, Peter Hurley wrote:
> >On 05/01/2014 03:04 PM, Murali Karicheri wrote:
> >>8250 uart driver currently supports only software assisted hw flow
> >>control. The software assisted hw flow control maintains a hw_stopped
> >>flag in the tty structure to stop and start transmission and use modem
> >>status interrupt for the event to drive the handshake signals. This is
> >>not needed if hw has flow control capabilities. This patch adds a
> >>DT attribute for enabling hw flow control for a uart port. Also skip
> >>stop and start if this flag is present in flag field of the port
> >>structure.
> >>
> >>Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
> >>
> >>CC: Rob Herring<robh+dt@kernel.org>
> >>CC: Pawel Moll<pawel.moll@arm.com>
> >>CC: Mark Rutland<mark.rutland@arm.com>
> >>CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
> >>CC: Kumar Gala<galak@codeaurora.org>
> >>CC: Randy Dunlap<rdunlap@infradead.org>
> >>CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> >>CC: Jiri Slaby<jslaby@suse.cz>
> >>CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
> >>---
> >>  .../devicetree/bindings/serial/of-serial.txt       |    1 +
> >>  drivers/tty/serial/8250/8250_core.c                |    6 ++++--
> >>  drivers/tty/serial/of_serial.c                     |    4 ++++
> >>  drivers/tty/serial/serial_core.c                   |   12 +++++++++---
> >>  4 files changed, 18 insertions(+), 5 deletions(-)
> >>
> >[...]
> >
> >>diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>index b68550d..851707a 100644
> >>--- a/drivers/tty/serial/serial_core.c
> >>+++ b/drivers/tty/serial/serial_core.c
> >>@@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> >>  			if (tty->termios.c_cflag&  CBAUD)
> >>  				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
> >>  		}
> >>-
> >>-		if (tty_port_cts_enabled(port)) {
> >>+		/*
> >>+		 * if hw support flow control without software intervention,
> >>+		 * then skip the below check
> >>+		 */
> >>+		if (tty_port_cts_enabled(port)&&
> >>+		    !(uport->flags&  UPF_HARD_FLOW)) {
> >>  			spin_lock_irq(&uport->lock);
> >>  			if (!(uport->ops->get_mctrl(uport)&  TIOCM_CTS))
> >>  				tty->hw_stopped = 1;
> >>@@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
> >>
> >>  	uport->icount.cts++;
> >>
> >>-	if (tty_port_cts_enabled(port)) {
> >>+	/* skip below code if the hw flow control is supported */
> >>+	if (tty_port_cts_enabled(port)&&
> >>+	    !(uport->flags&  UPF_HARD_FLOW)) {
> >Why is a modem status interrupt being generated for DCTS
> >if autoflow control is enabled?
> >
> >This should be:
> >
> >	WARN_ON_ONCE(uport->flags&  UPF_HARD_FLOW);
> >
> >to indicate a mis-configured driver/device.
> This patch is already merged to the upstream branch and if you see any
> issue, please
> post a patch for review.

If someone points out a problem in a patch of yours that is accepted
upstream, it is nice to provide a fix, otherwise I will just revert it
upstream, as it looks to be incorrect.

So, should I just revert it?

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Murali Karicheri Aug. 7, 2014, 5:24 p.m. UTC | #8
On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>> 8250 uart driver currently supports only software assisted hw flow
>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>> flag in the tty structure to stop and start transmission and use modem
>>>> status interrupt for the event to drive the handshake signals. This is
>>>> not needed if hw has flow control capabilities. This patch adds a
>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>> stop and start if this flag is present in flag field of the port
>>>> structure.
>>>>
>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>
>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>> ---
>>>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>>>
>>> [...]
>>>
>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>> index b68550d..851707a 100644
>>>> --- a/drivers/tty/serial/serial_core.c
>>>> +++ b/drivers/tty/serial/serial_core.c
>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>   			if (tty->termios.c_cflag&   CBAUD)
>>>>   				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>   		}
>>>> -
>>>> -		if (tty_port_cts_enabled(port)) {
>>>> +		/*
>>>> +		 * if hw support flow control without software intervention,
>>>> +		 * then skip the below check
>>>> +		 */
>>>> +		if (tty_port_cts_enabled(port)&&
>>>> +		    !(uport->flags&   UPF_HARD_FLOW)) {
>>>>   			spin_lock_irq(&uport->lock);
>>>>   			if (!(uport->ops->get_mctrl(uport)&   TIOCM_CTS))
>>>>   				tty->hw_stopped = 1;
>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>
>>>>   	uport->icount.cts++;
>>>>
>>>> -	if (tty_port_cts_enabled(port)) {
>>>> +	/* skip below code if the hw flow control is supported */
>>>> +	if (tty_port_cts_enabled(port)&&
>>>> +	    !(uport->flags&   UPF_HARD_FLOW)) {
>>> Why is a modem status interrupt being generated for DCTS
>>> if autoflow control is enabled?
>>>
>>> This should be:
>>>
>>> 	WARN_ON_ONCE(uport->flags&   UPF_HARD_FLOW);
>>>
>>> to indicate a mis-configured driver/device.
>> This patch is already merged to the upstream branch and if you see any
>> issue, please
>> post a patch for review.
>
> If someone points out a problem in a patch of yours that is accepted
> upstream, it is nice to provide a fix, otherwise I will just revert it
> upstream, as it looks to be incorrect.
>
> So, should I just revert it?
>
> greg k-h
Greg,

Sorry, I misunderstood it. I think it would have been better to start a 
new thread to point this out. Anyways I will send out a patch to add 
this and please don't revert.

Murali
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Hurley Aug. 7, 2014, 5:25 p.m. UTC | #9
On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>> 8250 uart driver currently supports only software assisted hw flow
>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>> flag in the tty structure to stop and start transmission and use modem
>>>> status interrupt for the event to drive the handshake signals. This is
>>>> not needed if hw has flow control capabilities. This patch adds a
>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>> stop and start if this flag is present in flag field of the port
>>>> structure.
>>>>
>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>
>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>  drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>  drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>  drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>  4 files changed, 18 insertions(+), 5 deletions(-)
>>>>
>>> [...]
>>>
>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>> index b68550d..851707a 100644
>>>> --- a/drivers/tty/serial/serial_core.c
>>>> +++ b/drivers/tty/serial/serial_core.c
>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>  			if (tty->termios.c_cflag&  CBAUD)
>>>>  				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>  		}
>>>> -
>>>> -		if (tty_port_cts_enabled(port)) {
>>>> +		/*
>>>> +		 * if hw support flow control without software intervention,
>>>> +		 * then skip the below check
>>>> +		 */
>>>> +		if (tty_port_cts_enabled(port)&&
>>>> +		    !(uport->flags&  UPF_HARD_FLOW)) {
>>>>  			spin_lock_irq(&uport->lock);
>>>>  			if (!(uport->ops->get_mctrl(uport)&  TIOCM_CTS))
>>>>  				tty->hw_stopped = 1;
>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>
>>>>  	uport->icount.cts++;
>>>>
>>>> -	if (tty_port_cts_enabled(port)) {
>>>> +	/* skip below code if the hw flow control is supported */
>>>> +	if (tty_port_cts_enabled(port)&&
>>>> +	    !(uport->flags&  UPF_HARD_FLOW)) {
>>> Why is a modem status interrupt being generated for DCTS
>>> if autoflow control is enabled?
>>>
>>> This should be:
>>>
>>> 	WARN_ON_ONCE(uport->flags&  UPF_HARD_FLOW);
>>>
>>> to indicate a mis-configured driver/device.
>> This patch is already merged to the upstream branch and if you see any
>> issue, please
>> post a patch for review.
> 
> If someone points out a problem in a patch of yours that is accepted
> upstream, it is nice to provide a fix, otherwise I will just revert it
> upstream, as it looks to be incorrect.
> 
> So, should I just revert it?

Greg,

As I look this patch over further, this should just be reverted.

1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
uart_unthrottle() is called.

2. The patch adds capabilities which already exist, namely UART_CAP_AFE.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Murali Karicheri Aug. 7, 2014, 5:34 p.m. UTC | #10
On 08/07/2014 01:25 PM, Peter Hurley wrote:
> On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
>> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>>> 8250 uart driver currently supports only software assisted hw flow
>>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>>> flag in the tty structure to stop and start transmission and use modem
>>>>> status interrupt for the event to drive the handshake signals. This is
>>>>> not needed if hw has flow control capabilities. This patch adds a
>>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>>> stop and start if this flag is present in flag field of the port
>>>>> structure.
>>>>>
>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>>
>>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>>> ---
>>>>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>> index b68550d..851707a 100644
>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>>   			if (tty->termios.c_cflag&   CBAUD)
>>>>>   				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>>   		}
>>>>> -
>>>>> -		if (tty_port_cts_enabled(port)) {
>>>>> +		/*
>>>>> +		 * if hw support flow control without software intervention,
>>>>> +		 * then skip the below check
>>>>> +		 */
>>>>> +		if (tty_port_cts_enabled(port)&&
>>>>> +		    !(uport->flags&   UPF_HARD_FLOW)) {
>>>>>   			spin_lock_irq(&uport->lock);
>>>>>   			if (!(uport->ops->get_mctrl(uport)&   TIOCM_CTS))
>>>>>   				tty->hw_stopped = 1;
>>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>>
>>>>>   	uport->icount.cts++;
>>>>>
>>>>> -	if (tty_port_cts_enabled(port)) {
>>>>> +	/* skip below code if the hw flow control is supported */
>>>>> +	if (tty_port_cts_enabled(port)&&
>>>>> +	    !(uport->flags&   UPF_HARD_FLOW)) {
>>>> Why is a modem status interrupt being generated for DCTS
>>>> if autoflow control is enabled?
>>>>
>>>> This should be:
>>>>
>>>> 	WARN_ON_ONCE(uport->flags&   UPF_HARD_FLOW);
>>>>
>>>> to indicate a mis-configured driver/device.
>>> This patch is already merged to the upstream branch and if you see any
>>> issue, please
>>> post a patch for review.
>>
>> If someone points out a problem in a patch of yours that is accepted
>> upstream, it is nice to provide a fix, otherwise I will just revert it
>> upstream, as it looks to be incorrect.
>>
>> So, should I just revert it?
>
> Greg,
>
> As I look this patch over further, this should just be reverted.

Sorry, I would suggest to fix it rather revert it.

>
> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
> uart_unthrottle() is called.
>
> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.
AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was 
described in my commit log as well where as this patch add support for 
pure h/w controlled flow  control and no software intervention is 
needed. Do you think uart_throttle() or  uart_unthrottle() is applicable
in this case?

Murali
>
> Regards,
> Peter Hurley
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Peter Hurley Aug. 7, 2014, 6:33 p.m. UTC | #11
On 08/07/2014 01:34 PM, Murali Karicheri wrote:
> On 08/07/2014 01:25 PM, Peter Hurley wrote:
>> On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>>>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>>>> 8250 uart driver currently supports only software assisted hw flow
>>>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>>>> flag in the tty structure to stop and start transmission and use modem
>>>>>> status interrupt for the event to drive the handshake signals. This is
>>>>>> not needed if hw has flow control capabilities. This patch adds a
>>>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>>>> stop and start if this flag is present in flag field of the port
>>>>>> structure.
>>>>>>
>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>>>
>>>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>>>> ---
>>>>>>   .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>>>   drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>>>   drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>>>   drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>>>   4 files changed, 18 insertions(+), 5 deletions(-)
>>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>>> index b68550d..851707a 100644
>>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>>>               if (tty->termios.c_cflag&   CBAUD)
>>>>>>                   uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>>>           }
>>>>>> -
>>>>>> -        if (tty_port_cts_enabled(port)) {
>>>>>> +        /*
>>>>>> +         * if hw support flow control without software intervention,
>>>>>> +         * then skip the below check
>>>>>> +         */
>>>>>> +        if (tty_port_cts_enabled(port)&&
>>>>>> +            !(uport->flags&   UPF_HARD_FLOW)) {
>>>>>>               spin_lock_irq(&uport->lock);
>>>>>>               if (!(uport->ops->get_mctrl(uport)&   TIOCM_CTS))
>>>>>>                   tty->hw_stopped = 1;
>>>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>>>
>>>>>>       uport->icount.cts++;
>>>>>>
>>>>>> -    if (tty_port_cts_enabled(port)) {
>>>>>> +    /* skip below code if the hw flow control is supported */
>>>>>> +    if (tty_port_cts_enabled(port)&&
>>>>>> +        !(uport->flags&   UPF_HARD_FLOW)) {
>>>>> Why is a modem status interrupt being generated for DCTS
>>>>> if autoflow control is enabled?
>>>>>
>>>>> This should be:
>>>>>
>>>>>     WARN_ON_ONCE(uport->flags&   UPF_HARD_FLOW);
>>>>>
>>>>> to indicate a mis-configured driver/device.
>>>> This patch is already merged to the upstream branch and if you see any
>>>> issue, please
>>>> post a patch for review.
>>>
>>> If someone points out a problem in a patch of yours that is accepted
>>> upstream, it is nice to provide a fix, otherwise I will just revert it
>>> upstream, as it looks to be incorrect.
>>>
>>> So, should I just revert it?
>>
>> Greg,
>>
>> As I look this patch over further, this should just be reverted.
> 
> Sorry, I would suggest to fix it rather revert it.
> 
>>
>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
>> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
>> uart_unthrottle() is called.
>>
>> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.

> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was described in my commit log as well where as this patch add support for pure h/w controlled flow  control and no software intervention is needed. Do you think uart_throttle() or  uart_unthrottle() is applicable
> in this case?

UART_CAP_AFE is used to indicate 16750-compatible hw flow control, which is
auto-CTS and auto-RTS flow control as described in the TI datasheet at
http://www.ti.com/lit/ds/symlink/tl16c750.pdf

uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
for high-level rx flow control, such as when a read buffer fills up because
there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
method because writing MCR to drop RTS is sufficient to disable auto-RTS.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri Aug. 7, 2014, 8:46 p.m. UTC | #12
On 08/07/2014 02:33 PM, Peter Hurley wrote:
> On 08/07/2014 01:34 PM, Murali Karicheri wrote:
>> On 08/07/2014 01:25 PM, Peter Hurley wrote:
>>> On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
>>>> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>>>>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>>>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>>>>> 8250 uart driver currently supports only software assisted hw flow
>>>>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>>>>> flag in the tty structure to stop and start transmission and use modem
>>>>>>> status interrupt for the event to drive the handshake signals. This is
>>>>>>> not needed if hw has flow control capabilities. This patch adds a
>>>>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>>>>> stop and start if this flag is present in flag field of the port
>>>>>>> structure.
>>>>>>>
>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>>>>
>>>>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>>>>    drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>>>>    drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>>>>    drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>>>>    4 files changed, 18 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>>>> index b68550d..851707a 100644
>>>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>>>>                if (tty->termios.c_cflag&    CBAUD)
>>>>>>>                    uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>>>>            }
>>>>>>> -
>>>>>>> -        if (tty_port_cts_enabled(port)) {
>>>>>>> +        /*
>>>>>>> +         * if hw support flow control without software intervention,
>>>>>>> +         * then skip the below check
>>>>>>> +         */
>>>>>>> +        if (tty_port_cts_enabled(port)&&
>>>>>>> +            !(uport->flags&    UPF_HARD_FLOW)) {
>>>>>>>                spin_lock_irq(&uport->lock);
>>>>>>>                if (!(uport->ops->get_mctrl(uport)&    TIOCM_CTS))
>>>>>>>                    tty->hw_stopped = 1;
>>>>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>>>>
>>>>>>>        uport->icount.cts++;
>>>>>>>
>>>>>>> -    if (tty_port_cts_enabled(port)) {
>>>>>>> +    /* skip below code if the hw flow control is supported */
>>>>>>> +    if (tty_port_cts_enabled(port)&&
>>>>>>> +        !(uport->flags&    UPF_HARD_FLOW)) {
>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>> if autoflow control is enabled?
>>>>>>
>>>>>> This should be:
>>>>>>
>>>>>>      WARN_ON_ONCE(uport->flags&    UPF_HARD_FLOW);
>>>>>>
>>>>>> to indicate a mis-configured driver/device.
>>>>> This patch is already merged to the upstream branch and if you see any
>>>>> issue, please
>>>>> post a patch for review.
>>>>
>>>> If someone points out a problem in a patch of yours that is accepted
>>>> upstream, it is nice to provide a fix, otherwise I will just revert it
>>>> upstream, as it looks to be incorrect.
>>>>
>>>> So, should I just revert it?
>>>
>>> Greg,
>>>
>>> As I look this patch over further, this should just be reverted.
>>
>> Sorry, I would suggest to fix it rather revert it.
>>
>>>
>>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
>>> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
>>> uart_unthrottle() is called.
>>>
>>> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.
>
>> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was described in my commit log as well where as this patch add support for pure h/w controlled flow  control and no software intervention is needed. Do you think uart_throttle() or  uart_unthrottle() is applicable
>> in this case?
>
> UART_CAP_AFE is used to indicate 16750-compatible hw flow control, which is
> auto-CTS and auto-RTS flow control as described in the TI datasheet at
> http://www.ti.com/lit/ds/symlink/tl16c750.pdf

Peter,

This patch was added to support hw flow control on boards based on 
Keystone SoCs that has UART with h/w flow control capability. Keystone 
SoCs UART spec is at http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
and it uses tl16550c as per the document. The equivalent spec seems to 
be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference 
between tl16c750 and tl16c550 seems to be 16 byte FIFO available instead 
of 16 or 64 byte FIFO in 16c750.

And about your original question
 >>>>>> Why is a modem status interrupt being generated for DCTS
 >>>>>> if autoflow control is enabled?

Are you asking why this patch didn't disable generating CTS interrupt 
when h/w flow control is enable? If so,

unsigned int serial8250_modem_status(struct uart_8250_port *up)
{
   ...
   		if (status & UART_MSR_DCTS)
			uart_handle_cts_change(port, status & UART_MSR_CTS);
}

Probably in the above check if UPF_HARD_FLOW is set, we can avoid 
calling uart_handle_cts_change() and undo the current code change in 
uart_handle_cts_change() and just add a WARN_ON_ONCE() as you have 
suggested.

>
> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
> for high-level rx flow control, such as when a read buffer fills up because
> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
> method because writing MCR to drop RTS is sufficient to disable auto-RTS.

As per spec. hardware has rx threshold levels set to trigger an RTS 
level change to tell the remote from sending more bytes. So if h/w flow 
control is enabled, then not sure why uart_throttle() is to be doing 
anything when h/w flow control is supported? A dummy function required 
to satisfy the line discipline?

Also 8250.c support other port types that doesn't have AFE. So shoudl 
this be driving RTS line to stop the sender and resume when 
uart_unthrottle() is called?

I want to work to fix this rather than revert this change as our 
customer is already using this feature. I will send out a patch based on 
this discussion.

Thanks and Regards,

Murali
>
> Regards,
> Peter Hurley
>

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Hurley Aug. 7, 2014, 11:03 p.m. UTC | #13
[ +cc Alan ]

On 08/07/2014 04:46 PM, Murali Karicheri wrote:
> On 08/07/2014 02:33 PM, Peter Hurley wrote:
>> On 08/07/2014 01:34 PM, Murali Karicheri wrote:
>>> On 08/07/2014 01:25 PM, Peter Hurley wrote:
>>>> On 08/07/2014 01:12 PM, Greg Kroah-Hartman wrote:
>>>>> On Thu, Aug 07, 2014 at 12:16:59PM -0400, Murali Karicheri wrote:
>>>>>> On 08/07/2014 11:29 AM, Peter Hurley wrote:
>>>>>>> On 05/01/2014 03:04 PM, Murali Karicheri wrote:
>>>>>>>> 8250 uart driver currently supports only software assisted hw flow
>>>>>>>> control. The software assisted hw flow control maintains a hw_stopped
>>>>>>>> flag in the tty structure to stop and start transmission and use modem
>>>>>>>> status interrupt for the event to drive the handshake signals. This is
>>>>>>>> not needed if hw has flow control capabilities. This patch adds a
>>>>>>>> DT attribute for enabling hw flow control for a uart port. Also skip
>>>>>>>> stop and start if this flag is present in flag field of the port
>>>>>>>> structure.
>>>>>>>>
>>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>>>>>>>>
>>>>>>>> CC: Rob Herring<robh+dt@kernel.org>
>>>>>>>> CC: Pawel Moll<pawel.moll@arm.com>
>>>>>>>> CC: Mark Rutland<mark.rutland@arm.com>
>>>>>>>> CC: Ian Campbell<ijc+devicetree@hellion.org.uk>
>>>>>>>> CC: Kumar Gala<galak@codeaurora.org>
>>>>>>>> CC: Randy Dunlap<rdunlap@infradead.org>
>>>>>>>> CC: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>>>>> CC: Jiri Slaby<jslaby@suse.cz>
>>>>>>>> CC: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/serial/of-serial.txt       |    1 +
>>>>>>>>    drivers/tty/serial/8250/8250_core.c                |    6 ++++--
>>>>>>>>    drivers/tty/serial/of_serial.c                     |    4 ++++
>>>>>>>>    drivers/tty/serial/serial_core.c                   |   12 +++++++++---
>>>>>>>>    4 files changed, 18 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>>>>> index b68550d..851707a 100644
>>>>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>>>>> @@ -174,8 +174,12 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>>>>>>>>                if (tty->termios.c_cflag&    CBAUD)
>>>>>>>>                    uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
>>>>>>>>            }
>>>>>>>> -
>>>>>>>> -        if (tty_port_cts_enabled(port)) {
>>>>>>>> +        /*
>>>>>>>> +         * if hw support flow control without software intervention,
>>>>>>>> +         * then skip the below check
>>>>>>>> +         */
>>>>>>>> +        if (tty_port_cts_enabled(port)&&
>>>>>>>> +            !(uport->flags&    UPF_HARD_FLOW)) {
>>>>>>>>                spin_lock_irq(&uport->lock);
>>>>>>>>                if (!(uport->ops->get_mctrl(uport)&    TIOCM_CTS))
>>>>>>>>                    tty->hw_stopped = 1;
>>>>>>>> @@ -2772,7 +2776,9 @@ void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>>>>>>>>
>>>>>>>>        uport->icount.cts++;
>>>>>>>>
>>>>>>>> -    if (tty_port_cts_enabled(port)) {
>>>>>>>> +    /* skip below code if the hw flow control is supported */
>>>>>>>> +    if (tty_port_cts_enabled(port)&&
>>>>>>>> +        !(uport->flags&    UPF_HARD_FLOW)) {
>>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>>> if autoflow control is enabled?
>>>>>>>
>>>>>>> This should be:
>>>>>>>
>>>>>>>      WARN_ON_ONCE(uport->flags&    UPF_HARD_FLOW);
>>>>>>>
>>>>>>> to indicate a mis-configured driver/device.
>>>>>> This patch is already merged to the upstream branch and if you see any
>>>>>> issue, please
>>>>>> post a patch for review.
>>>>>
>>>>> If someone points out a problem in a patch of yours that is accepted
>>>>> upstream, it is nice to provide a fix, otherwise I will just revert it
>>>>> upstream, as it looks to be incorrect.
>>>>>
>>>>> So, should I just revert it?
>>>>
>>>> Greg,
>>>>
>>>> As I look this patch over further, this should just be reverted.
>>>
>>> Sorry, I would suggest to fix it rather revert it.
>>>
>>>>
>>>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
>>>> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
>>>> uart_unthrottle() is called.
>>>>
>>>> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.
>>
>>> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was described in my commit log as well where as this patch add support for pure h/w controlled flow  control and no software intervention is needed. Do you think uart_throttle() or  uart_unthrottle() is applicable
>>> in this case?
>>
>> UART_CAP_AFE is used to indicate 16750-compatible hw flow control, which is
>> auto-CTS and auto-RTS flow control as described in the TI datasheet at
>> http://www.ti.com/lit/ds/symlink/tl16c750.pdf
> 
> Peter,
> 
> This patch was added to support hw flow control on boards based on Keystone SoCs that has UART with h/w flow control capability. Keystone SoCs UART spec is at http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
> and it uses tl16550c as per the document. The equivalent spec seems to be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference between tl16c750 and tl16c550 seems to be 16 byte FIFO available instead of 16 or 64 byte FIFO in 16c750.

So this is just a way to avoid the fifo size check.
Then I'd rather see a capability that only overrides the fifo size check
and does not enable UPF_HARD_FLOW.

Or show that the fifo size check is not actually required for AFE.


> And about your original question
>>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>>> if autoflow control is enabled?
> 
> Are you asking why this patch didn't disable generating CTS interrupt when h/w flow control is enable?

No. I was asking what chip had AFE on but still generated modem status
interrupts for delta CTS.

But I realize now that a different question needs asking:
Is the MSR read showing delta CTS set when AFE is on, ever?

Because serial8250_modem_status() assumes the answer is no for
_all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
is broken if AFE is on.


> If so,
> 
> unsigned int serial8250_modem_status(struct uart_8250_port *up)
> {
>   ...
>           if (status & UART_MSR_DCTS)
>             uart_handle_cts_change(port, status & UART_MSR_CTS);
> }
> 
> Probably in the above check if UPF_HARD_FLOW is set, we can avoid calling uart_handle_cts_change() and undo the current code change in uart_handle_cts_change() and just add a WARN_ON_ONCE() as you have suggested.

Let's come back to this question after determining the answer for the
above question.


>>
>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>> for high-level rx flow control, such as when a read buffer fills up because
>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
> 
> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell the remote from sending more bytes. So if h/w flow control is enabled, then not sure why uart_throttle() is to be doing anything when h/w flow control is supported? A dummy function required to satisfy the line discipline?

I understand how auto-RTS works.

Let's pretend for a moment that uart_throttle() does nothing when
auto-RTS is enabled:

1. tty buffers start to fill up because no process is reading the data.
2. The throttle threshold is reached.
3. uart_throttle() is called but does nothing.
4. more data arrives and the DR interrupt is triggered
5. serial8250_rx_chars() reads in the new data.
6. tty buffers keep filling up even though the driver was told to throttle
7. eventually the tty buffers will cap at about 64KB and start counting
   buf_overrun errors


> Also 8250.c support other port types that doesn't have AFE. So shoudl this be driving RTS line to stop the sender and resume when uart_unthrottle() is called?

Yes, because that's how sw-assisted CTS flow control works, and the
default behavior of uart_throttle/uart_unthrottle.

IOW, with no extra code or special-casing, AFE just works.

> I want to work to fix this rather than revert this change as our customer is already using this feature.

3.16 was released 4 days ago.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri Aug. 8, 2014, 7:36 p.m. UTC | #14
On 08/07/2014 07:03 PM, Peter Hurley wrote:

----Cut-------------
>>>>
>>>>>
>>>>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and unthrottle()
>>>>> methods for 8250, which is guaranteed to blow-up when either uart_throttle() or
>>>>> uart_unthrottle() is called.
>>>>>
>>>>> 2. The patch adds capabilities which already exist, namely UART_CAP_AFE.
>>>
>>>> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it was described in my commit log as well where as this patch add support for pure h/w controlled flow  control and no software intervention is needed. Do you think uart_throttle() or  uart_unthrottle() is applicable
>>>> in this case?
>>>
>>> UART_CAP_AFE is used to indicate 16750-compatible hw flow control, which is
>>> auto-CTS and auto-RTS flow control as described in the TI datasheet at
>>> http://www.ti.com/lit/ds/symlink/tl16c750.pdf
>>
>> Peter,
>>
>> This patch was added to support hw flow control on boards based on Keystone SoCs that has UART with h/w flow control capability. Keystone SoCs UART spec is at http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
>> and it uses tl16550c as per the document. The equivalent spec seems to be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference between tl16c750 and tl16c550 seems to be 16 byte FIFO available instead of 16 or 64 byte FIFO in 16c750.
>
> So this is just a way to avoid the fifo size check.
> Then I'd rather see a capability that only overrides the fifo size check
> and does not enable UPF_HARD_FLOW.
>
> Or show that the fifo size check is not actually required for AFE.
>
>
>> And about your original question
>>>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>>>> if autoflow control is enabled?
>>
>> Are you asking why this patch didn't disable generating CTS interrupt when h/w flow control is enable?
>
> No. I was asking what chip had AFE on but still generated modem status
> interrupts for delta CTS.
>
> But I realize now that a different question needs asking:
> Is the MSR read showing delta CTS set when AFE is on, ever?

Unfortunately this was tested on a customer board that I don't have 
access to and can't check this out right away. I am trying to findout 
if I can get some hardware to test the patch to address the issue being 
discussed. Customer board is currently using RTS and CTS lines and the 
same works fine for them with this patch.

>
> Because serial8250_modem_status() assumes the answer is no for
> _all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
> is broken if AFE is on.

As per Keystone UART spec

bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the 
CTS input has changed state since the last time it was read by the CPU. 
When DCTS is set (autoflow control is not enabled and the modem status 
interrupt is enabled), a modem status interrupt is generated. When 
autoflow control is enabled, no interrupt is generated

So based on this, there shouldn't be any CTS change if AFE is enabled 
and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE() 
as you suggested to detect any offending h/w.

>
>
>> If so,
>>
>> unsigned int serial8250_modem_status(struct uart_8250_port *up)
>> {
>>    ...
>>            if (status&  UART_MSR_DCTS)
>>              uart_handle_cts_change(port, status&  UART_MSR_CTS);
>> }
>>
>> Probably in the above check if UPF_HARD_FLOW is set, we can avoid calling
 >> uart_handle_cts_change() and undo the current code change in 
uart_handle_cts_change()
 >> and just add a WARN_ON_ONCE() as you have suggested.
>
> Let's come back to this question after determining the answer for the
> above question.
>
>
>>>
>>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>>> for high-level rx flow control, such as when a read buffer fills up because
>>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
>>
>> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell
 >> the remote from sending more bytes. So if h/w flow control is 
enabled, then not sure why
 >> uart_throttle() is to be doing anything when h/w flow control is 
supported? A dummy
 >> function required to satisfy the line discipline?
>
> I understand how auto-RTS works.
>
> Let's pretend for a moment that uart_throttle() does nothing when
> auto-RTS is enabled:
>
> 1. tty buffers start to fill up because no process is reading the data.
> 2. The throttle threshold is reached.
> 3. uart_throttle() is called but does nothing.
> 4. more data arrives and the DR interrupt is triggered
> 5. serial8250_rx_chars() reads in the new data.
> 6. tty buffers keep filling up even though the driver was told to throttle
> 7. eventually the tty buffers will cap at about 64KB and start counting
>     buf_overrun errors
>
Ok.

Couple of observation on the AFE implementation in 8250 driver prior to 
my patch.

 From the discussion so far, AFE is actually hardware assisted 
hardware flow control. Auto CTS is sw assisted hardware flow control
where sw uses RTS line for recieve side flow control and I assume it 
uses MCR RTS bit for this where AFE does this automatically. From
the 16550 or Keystone UART spec, I can't find how RTS line can be 
asserted to do this through sw instead of hardware doing it 
automatically. Spec says

MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
e autoflow control enabled. Note that all UARTs do not support this 
feature. See the device-specific data manual for supported features. If 
this feature is not available, this bit is reserved and should be 
cleared to 0.
0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
1 = UARTn_RTS and UARTn_CTS are enabled.

Then since AFE was already supported before my patch for FIFO size 
32bytes or higher, I am wondering why there was no implementation of 
throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not 
set at all if AFE implemented in 8250 driver is hw assisted, hw flow 
control. Also what do these API supposed to do?

in serial_core.c, uart_throttle() function

  if CRTSCTS is set
      port->ops->throttle(port);
  Also call at the end
      uart_clear_mctrl(port, TIOCM_RTS);
  and this go an clear the MCR RTS bit.

So what does uart_throttle() API expected to do since MCR is updated 
using uart_clear_mctrl().

I searched who sets the UPF_HARD_FLOW in port->flags and only driver 
that does set this flag is omap-serial.c. The check was introduced by 
commit from Ruessel King to support h/w assisted h/w flow control.

======================================================================
commit dba05832cbe4f305dfd998fb26d7c685d91fbbd8
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Tue Apr 17 16:41:10 2012 +0100

     SERIAL: core: add hardware assisted h/w flow control support

     Ports which are handling h/w flow control in hardware must not have
     their RTS state altered depending on the tty's hardware-stopped state.
     Avoid this additional logic when setting the termios state.

     Acked-by: Alan Cox <alan@linux.intel.com>
     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
======================================================================

This flag is checked in uart_set_termios() and the reason is in the 
commit log above.

So shouldn't AFE support in 8250 make use of this flag? My patch uses DT 
attribute to set this flag so above uart_set_termios() function behave 
as expected. But as you have rightly pointed out throttle()/unthrottle() 
is missing that needs to be added. Only driver that I can find that has 
implemented this is omap_serial.c. It does disable RDI and RLSI as part 
of throttle() interrupt and re-enable it as part of unthrottle(). 
Probably I can add this in this driver as well if this is what is 
expected. I will post a patch for this anyways, with some basic testing, 
but testing on customer h/w for this was initialially implemented has to 
wait until my return from vacation  (on vacation from 08/11-08/16).

>
>> Also 8250.c support other port types that doesn't have AFE. So shoudl this
 >> be driving RTS line to stop the sender and resume when 
uart_unthrottle() is called?
>
> Yes, because that's how sw-assisted CTS flow control works, and the
> default behavior of uart_throttle/uart_unthrottle.
>
> IOW, with no extra code or special-casing, AFE just works.
>

Based on my above discussion, there are few things required to be done 
on top of AFE and some of it is done by my patch and the remaining thing 
to be addressed in another patch.

>> I want to work to fix this rather than revert this change as our customer is already using this feature.
>
> 3.16 was released 4 days ago.

As I said, I will work to address this with priority.

>
> Regards,
> Peter Hurley
>

--
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
Peter Hurley Aug. 8, 2014, 8:44 p.m. UTC | #15
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
On 08/08/2014 03:36 PM, Murali Karicheri wrote:
> On 08/07/2014 07:03 PM, Peter Hurley wrote:

[...]

>> But I realize now that a different question needs asking:
>> Is the MSR read showing delta CTS set when AFE is on, ever?
> 
> Unfortunately this was tested on a customer board that I don't have access to and can't check this out right away. I am trying to findout if I can get some hardware to test the patch to address the issue being discussed. Customer board is currently using RTS and CTS lines and the same works fine for them with this patch.

Ok.


>> Because serial8250_modem_status() assumes the answer is no for
>> _all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
>> is broken if AFE is on.
> 
> As per Keystone UART spec
> 
> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated
> 
> So based on this, there shouldn't be any CTS change if AFE is enabled and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE() as you suggested to detect any offending h/w.

That's identical wording to the 16750 datasheet.

But notice that it only says "no interrupt is generated" when AFE is on.
It doesn't say if the MSR is read, that DCTS won't be set. And that's
an important difference for how serial8250_modem_status() works.

[...]


>>>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>>>> for high-level rx flow control, such as when a read buffer fills up because
>>>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>>>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
>>>
>>> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell
>>> the remote from sending more bytes. So if h/w flow control is enabled, then not sure why
>>> uart_throttle() is to be doing anything when h/w flow control is supported? A dummy
>>> function required to satisfy the line discipline?
>>
>> I understand how auto-RTS works.
>>
>> Let's pretend for a moment that uart_throttle() does nothing when
>> auto-RTS is enabled:
>>
>> 1. tty buffers start to fill up because no process is reading the data.
>> 2. The throttle threshold is reached.
>> 3. uart_throttle() is called but does nothing.
>> 4. more data arrives and the DR interrupt is triggered
>> 5. serial8250_rx_chars() reads in the new data.
>> 6. tty buffers keep filling up even though the driver was told to throttle
>> 7. eventually the tty buffers will cap at about 64KB and start counting
>>     buf_overrun errors
>>
> Ok.
> 
> Couple of observation on the AFE implementation in 8250 driver prior to my patch.
> 
> From the discussion so far, AFE is actually hardware assisted hardware flow control. Auto CTS is sw assisted hardware flow control
> where sw uses RTS line for recieve side flow control and I assume it uses MCR RTS bit for this where AFE does this automatically. From
> the 16550 or Keystone UART spec, I can't find how RTS line can be asserted to do this through sw instead of hardware doing it automatically. Spec says
>
> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
> e autoflow control enabled. Note that all UARTs do not support this feature. See the device-specific data manual for supported features. If this feature is not available, this bit is reserved and should be cleared to 0.
> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
> 1 = UARTn_RTS and UARTn_CTS are enabled.
> 
> Then since AFE was already supported before my patch for FIFO size 32bytes or higher, I am wondering why there was no implementation of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not set at all if AFE implemented in 8250 driver is hw assisted, hw flow control. Also what do these API supposed to do?

uart_throttle() does _not_ call ops->throttle() unless either
UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.

So with AFE, UPF_HARD_FLOW is not set (even though AFE is hw flow control),
because no special throttle/unthrottle is required; the default action of
uart_throttle() suffices.

With AFE, as long as the MCR RTS bit is set by software, auto-RTS is functioning;
ie., the receiver FIFO level is controlling the state of the RTS output.
If uart_throttle() is called, the MCR RTS bit is cleared, which overrides the
auto-RTS control and deasserts the RTS output.

When uart_unthrottle() is called, the MCR RTS bit is set, which re-enables the
auto-RTS function.


> in serial_core.c, uart_throttle() function
> 
>  if CRTSCTS is set
>      port->ops->throttle(port);
       ^^^^^^^^^^^^^^^^^^^^^^^^^
       not called in 8250 driver because UPF_SOFT_FLOW and UPF_HARD_FLOW are both unset
>  Also call at the end
>      uart_clear_mctrl(port, TIOCM_RTS);
>  and this go an clear the MCR RTS bit.
                  ^^^^^^^^^^^^^^^^^^^^^
                  which turns off auto-RTS

When uart_unthrottle() reenables MCR RTS, then auto-RTS kicks back on.


> So what does uart_throttle() API expected to do since MCR is updated using uart_clear_mctrl().
> 
> I searched who sets the UPF_HARD_FLOW in port->flags and only driver that does set this flag is omap-serial.c. The check was introduced by commit from Ruessel King to support h/w assisted h/w flow control.
>
> ======================================================================
> commit dba05832cbe4f305dfd998fb26d7c685d91fbbd8
> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date:   Tue Apr 17 16:41:10 2012 +0100
> 
>     SERIAL: core: add hardware assisted h/w flow control support
> 
>     Ports which are handling h/w flow control in hardware must not have
>     their RTS state altered depending on the tty's hardware-stopped state.
>     Avoid this additional logic when setting the termios state.
> 
>     Acked-by: Alan Cox <alan@linux.intel.com>

> ======================================================================
> 
> This flag is checked in uart_set_termios() and the reason is in the commit log above.
> 
> So shouldn't AFE support in 8250 make use of this flag?

No. Hopefully my explanation of how the 8250 driver uses AFE clears that up.


> My patch uses DT attribute to set this flag so above uart_set_termios() function behave as expected. But as you have rightly pointed out throttle()/unthrottle() is missing that needs to be added. Only driver that I can find that has implemented this is omap_serial.c. It does disable RDI and RLSI as part of throttle() interrupt and re-enable it as part of unthrottle(). Probably I can add this in this driver as well if this is what is expected. I will post a patch for this anyways, with some basic testing, but testing on customer h/w for this was initialially implemented has to wait until my return from vacation  (on vacation from 08/11-08/16).

Which is why we should revert the previous patch, and restart the effort with
a patch that adds a way to disable the fifo level check and see if that just
works with your customer hardware (with UART_CAP_AFE set in the firmware).


>>> Also 8250.c support other port types that doesn't have AFE. So shoudl this
>>> be driving RTS line to stop the sender and resume when uart_unthrottle() is called?
>>
>> Yes, because that's how sw-assisted CTS flow control works, and the
>> default behavior of uart_throttle/uart_unthrottle.
>>
>> IOW, with no extra code or special-casing, AFE just works.
>>
> 
> Based on my above discussion, there are few things required to be done on top of AFE and some of it is done by my patch and the remaining thing to be addressed in another patch.

Assuming that AFE, as already implemented in the 8250 driver, works as expected,
the fifo level check seems to be the only hurdle, right?


>>> I want to work to fix this rather than revert this change as our customer is already using this feature.
>>
>> 3.16 was released 4 days ago.
> 
> As I said, I will work to address this with priority.
 
My point was that I'm not understanding how your customer could be using this
feature when it came out 4 days ago, but yet now you can't even test on the
hardware?

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Murali Karicheri Aug. 8, 2014, 8:46 p.m. UTC | #16
On 08/08/2014 03:36 PM, Murali Karicheri wrote:
> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>
> ----Cut-------------
>>>>>
>>>>>>
>>>>>> 1. The patch enables UPF_HARD_FLOW, but provides no throttle() and
>>>>>> unthrottle()
>>>>>> methods for 8250, which is guaranteed to blow-up when either
>>>>>> uart_throttle() or
>>>>>> uart_unthrottle() is called.
>>>>>>
>>>>>> 2. The patch adds capabilities which already exist, namely
>>>>>> UART_CAP_AFE.
>>>>
>>>>> AFAIK, UART_CAP_AFE is a software assisted hw flow control and it
>>>>> was described in my commit log as well where as this patch add
>>>>> support for pure h/w controlled flow control and no software
>>>>> intervention is needed. Do you think uart_throttle() or
>>>>> uart_unthrottle() is applicable
>>>>> in this case?
>>>>
>>>> UART_CAP_AFE is used to indicate 16750-compatible hw flow control,
>>>> which is
>>>> auto-CTS and auto-RTS flow control as described in the TI datasheet at
>>>> http://www.ti.com/lit/ds/symlink/tl16c750.pdf
>>>
>>> Peter,
>>>
>>> This patch was added to support hw flow control on boards based on
>>> Keystone SoCs that has UART with h/w flow control capability.
>>> Keystone SoCs UART spec is at
>>> http://www.ti.com/lit/ug/sprugp1/sprugp1.pdf
>>> and it uses tl16550c as per the document. The equivalent spec seems
>>> to be http://www.ti.com/lit/ds/symlink/tl16c550c.pdf. Only difference
>>> between tl16c750 and tl16c550 seems to be 16 byte FIFO available
>>> instead of 16 or 64 byte FIFO in 16c750.
>>
>> So this is just a way to avoid the fifo size check.
>> Then I'd rather see a capability that only overrides the fifo size check
>> and does not enable UPF_HARD_FLOW.
>>
>> Or show that the fifo size check is not actually required for AFE.
>>
>>
>>> And about your original question
>>>>>>>>> Why is a modem status interrupt being generated for DCTS
>>>>>>>>> if autoflow control is enabled?
>>>
>>> Are you asking why this patch didn't disable generating CTS interrupt
>>> when h/w flow control is enable?
>>
>> No. I was asking what chip had AFE on but still generated modem status
>> interrupts for delta CTS.
>>
>> But I realize now that a different question needs asking:
>> Is the MSR read showing delta CTS set when AFE is on, ever?
>
> Unfortunately this was tested on a customer board that I don't have
> access to and can't check this out right away. I am trying to findout if
> I can get some hardware to test the patch to address the issue being
> discussed. Customer board is currently using RTS and CTS lines and the
> same works fine for them with this patch.
>
>>
>> Because serial8250_modem_status() assumes the answer is no for
>> _all_ AFE-capable devices, and if yes, would mean that
>> serial8250_modem_status()
>> is broken if AFE is on.
>
> As per Keystone UART spec
>
> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the
> CTS input has changed state since the last time it was read by the CPU.
> When DCTS is set (autoflow control is not enabled and the modem status
> interrupt is enabled), a modem status interrupt is generated. When
> autoflow control is enabled, no interrupt is generated
>
> So based on this, there shouldn't be any CTS change if AFE is enabled
> and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE()
> as you suggested to detect any offending h/w.
>
>>
>>
>>> If so,
>>>
>>> unsigned int serial8250_modem_status(struct uart_8250_port *up)
>>> {
>>> ...
>>> if (status& UART_MSR_DCTS)
>>> uart_handle_cts_change(port, status& UART_MSR_CTS);
>>> }
>>>
>>> Probably in the above check if UPF_HARD_FLOW is set, we can avoid
>>> calling
>  >> uart_handle_cts_change() and undo the current code change in
> uart_handle_cts_change()
>  >> and just add a WARN_ON_ONCE() as you have suggested.
>>
>> Let's come back to this question after determining the answer for the
>> above question.
>>
>>
>>>>
>>>> uart_throttle() and uart_unthrottle() are used indirectly by line
>>>> disciplines
>>>> for high-level rx flow control, such as when a read buffer fills up
>>>> because
>>>> there is no userspace reader. The 8250 core doesn't define a
>>>> throttle/unthrottle
>>>> method because writing MCR to drop RTS is sufficient to disable
>>>> auto-RTS.
>>>
>>> As per spec. hardware has rx threshold levels set to trigger an RTS
>>> level change to tell
>  >> the remote from sending more bytes. So if h/w flow control is
> enabled, then not sure why
>  >> uart_throttle() is to be doing anything when h/w flow control is
> supported? A dummy
>  >> function required to satisfy the line discipline?
>>
>> I understand how auto-RTS works.
>>
>> Let's pretend for a moment that uart_throttle() does nothing when
>> auto-RTS is enabled:
>>
>> 1. tty buffers start to fill up because no process is reading the data.
>> 2. The throttle threshold is reached.
>> 3. uart_throttle() is called but does nothing.
>> 4. more data arrives and the DR interrupt is triggered
>> 5. serial8250_rx_chars() reads in the new data.
>> 6. tty buffers keep filling up even though the driver was told to
>> throttle
>> 7. eventually the tty buffers will cap at about 64KB and start counting
>> buf_overrun errors
>>
> Ok.
>
> Couple of observation on the AFE implementation in 8250 driver prior to
> my patch.
>
>  From the discussion so far, AFE is actually hardware assisted hardware
> flow control. Auto CTS is sw assisted hardware flow control
> where sw uses RTS line for recieve side flow control and I assume it
> uses MCR RTS bit for this where AFE does this automatically. From
> the 16550 or Keystone UART spec, I can't find how RTS line can be
> asserted to do this through sw instead of hardware doing it
> automatically. Spec says
>
> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
> e autoflow control enabled. Note that all UARTs do not support this
> feature. See the device-specific data manual for supported features. If
> this feature is not available, this bit is reserved and should be
> cleared to 0.
> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
> 1 = UARTn_RTS and UARTn_CTS are enabled.
>
> Then since AFE was already supported before my patch for FIFO size
> 32bytes or higher, I am wondering why there was no implementation of
> throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not
> set at all if AFE implemented in 8250 driver is hw assisted, hw flow
> control. Also what do these API supposed to do?
>
> in serial_core.c, uart_throttle() function
>
> if CRTSCTS is set
> port->ops->throttle(port);
> Also call at the end
> uart_clear_mctrl(port, TIOCM_RTS);
> and this go an clear the MCR RTS bit.
>
> So what does uart_throttle() API expected to do since MCR is updated
> using uart_clear_mctrl().
>
> I searched who sets the UPF_HARD_FLOW in port->flags and only driver
> that does set this flag is omap-serial.c. The check was introduced by
> commit from Ruessel King to support h/w assisted h/w flow control.
>
> ======================================================================
> commit dba05832cbe4f305dfd998fb26d7c685d91fbbd8
> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date: Tue Apr 17 16:41:10 2012 +0100
>
> SERIAL: core: add hardware assisted h/w flow control support
>
> Ports which are handling h/w flow control in hardware must not have
> their RTS state altered depending on the tty's hardware-stopped state.
> Avoid this additional logic when setting the termios state.
>
> Acked-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ======================================================================
>
> This flag is checked in uart_set_termios() and the reason is in the
> commit log above.
>
> So shouldn't AFE support in 8250 make use of this flag? My patch uses DT
> attribute to set this flag so above uart_set_termios() function behave
> as expected. But as you have rightly pointed out throttle()/unthrottle()
> is missing that needs to be added. Only driver that I can find that has
> implemented this is omap_serial.c. It does disable RDI and RLSI as part
> of throttle() interrupt and re-enable it as part of unthrottle().
> Probably I can add this in this driver as well if this is what is
> expected. I will post a patch for this anyways, with some basic testing,
> but testing on customer h/w for this was initialially implemented has to
> wait until my return from vacation (on vacation from 08/11-08/16).
>
>>
>>> Also 8250.c support other port types that doesn't have AFE. So shoudl
>>> this
>  >> be driving RTS line to stop the sender and resume when
> uart_unthrottle() is called?
>>
>> Yes, because that's how sw-assisted CTS flow control works, and the
>> default behavior of uart_throttle/uart_unthrottle.
>>
>> IOW, with no extra code or special-casing, AFE just works.
>>
>
> Based on my above discussion, there are few things required to be done
> on top of AFE and some of it is done by my patch and the remaining thing
> to be addressed in another patch.
>
>>> I want to work to fix this rather than revert this change as our
>>> customer is already using this feature.
>>
>> 3.16 was released 4 days ago.
>
> As I said, I will work to address this with priority.
Peter, Greg,

I have send out an RFC "serial: uart: update to support hw assisted hw 
flow control" to address this issue based on this thread.
I did only some basic testing on this and want to get your feedback.
I will work to test this patch on our customer board once I return from 
my vacation. Please review and give your comments..

Thanks and regards,

Murali Karicheri

>
>>
>> Regards,
>> Peter Hurley
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri Aug. 8, 2014, 9:02 p.m. UTC | #17
On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>      Signed-off-by: Russell King<rmk+kernel@arm.linux.org.uk>
> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>
> [...]
>
>>> But I realize now that a different question needs asking:
>>> Is the MSR read showing delta CTS set when AFE is on, ever?
>>
>> Unfortunately this was tested on a customer board that I don't have access to and can't check this out right away. I am trying to findout if I can get some hardware to test the patch to address the issue being discussed. Customer board is currently using RTS and CTS lines and the same works fine for them with this patch.
>
> Ok.
>
>
>>> Because serial8250_modem_status() assumes the answer is no for
>>> _all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
>>> is broken if AFE is on.
>>
>> As per Keystone UART spec
>>
>> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated
>>
>> So based on this, there shouldn't be any CTS change if AFE is enabled and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE() as you suggested to detect any offending h/w.
>
> That's identical wording to the 16750 datasheet.
>
> But notice that it only says "no interrupt is generated" when AFE is on.
> It doesn't say if the MSR is read, that DCTS won't be set. And that's
> an important difference for how serial8250_modem_status() works.
>
> [...]
>
>
>>>>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>>>>> for high-level rx flow control, such as when a read buffer fills up because
>>>>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>>>>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
>>>>
>>>> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell
>>>> the remote from sending more bytes. So if h/w flow control is enabled, then not sure why
>>>> uart_throttle() is to be doing anything when h/w flow control is supported? A dummy
>>>> function required to satisfy the line discipline?
>>>
>>> I understand how auto-RTS works.
>>>
>>> Let's pretend for a moment that uart_throttle() does nothing when
>>> auto-RTS is enabled:
>>>
>>> 1. tty buffers start to fill up because no process is reading the data.
>>> 2. The throttle threshold is reached.
>>> 3. uart_throttle() is called but does nothing.
>>> 4. more data arrives and the DR interrupt is triggered
>>> 5. serial8250_rx_chars() reads in the new data.
>>> 6. tty buffers keep filling up even though the driver was told to throttle
>>> 7. eventually the tty buffers will cap at about 64KB and start counting
>>>      buf_overrun errors
>>>
>> Ok.
>>
>> Couple of observation on the AFE implementation in 8250 driver prior to my patch.
>>
>>  From the discussion so far, AFE is actually hardware assisted hardware flow control. Auto CTS is sw assisted hardware flow control
>> where sw uses RTS line for recieve side flow control and I assume it uses MCR RTS bit for this where AFE does this automatically. From
>> the 16550 or Keystone UART spec, I can't find how RTS line can be asserted to do this through sw instead of hardware doing it automatically. Spec says
>>
>> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
>> e autoflow control enabled. Note that all UARTs do not support this feature. See the device-specific data manual for supported features. If this feature is not available, this bit is reserved and should be cleared to 0.
>> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
>> 1 = UARTn_RTS and UARTn_CTS are enabled.
>>
>> Then since AFE was already supported before my patch for FIFO size 32bytes or higher, I am wondering why there was no implementation of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not set at all if AFE implemented in 8250 driver is hw assisted, hw flow control. Also what do these API supposed to do?
>
> uart_throttle() does _not_ call ops->throttle() unless either
> UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.
>

Not based on port flag. Here is the actual code in serial_core.c as I 
see it.

static void uart_throttle(struct tty_struct *tty)
{
	struct uart_state *state = tty->driver_data;
	struct uart_port *port = state->uart_port;
	uint32_t mask = 0;

	if (I_IXOFF(tty))
		mask |= UPF_SOFT_FLOW;
	if (tty->termios.c_cflag & CRTSCTS)
		mask |= UPF_HARD_FLOW;

	if (port->flags & mask) {
		port->ops->throttle(port);
		mask &= ~port->flags;
	}

	if (mask & UPF_SOFT_FLOW)
		uart_send_xchar(tty, STOP_CHAR(tty));

	if (mask & UPF_HARD_FLOW)
		uart_clear_mctrl(port, TIOCM_RTS);
}


As you see it is based on tty->termios.c_cflag. Even for AFE enabled 
case before my patch if this flag is  set, it is going to call 
throttle() which will crash for 8250.c

> So with AFE, UPF_HARD_FLOW is not set (even though AFE is hw flow control),
> because no special throttle/unthrottle is required; the default action of
> uart_throttle() suffices.

This is not true as described in my response. So this can crash even 
before my patch was introduced unless we don't refer the same code :)
The 8250.c driver sets UART_MCR_AFE when termios->c_cflag & CRTSCTS is 
set and same is checked in throttle(). So for throttle, we might want to 
do nothing for AFE and probably disable RDI and RLSI interrupts as done 
in omap serial driver (omap-serial.c) ?

>
> With AFE, as long as the MCR RTS bit is set by software, auto-RTS is functioning;
> ie., the receiver FIFO level is controlling the state of the RTS output.
> If uart_throttle() is called, the MCR RTS bit is cleared, which overrides the
> auto-RTS control and deasserts the RTS output.
>
> When uart_unthrottle() is called, the MCR RTS bit is set, which re-enables the
> auto-RTS function.
>
>
>> in serial_core.c, uart_throttle() function
>>
>>   if CRTSCTS is set
>>       port->ops->throttle(port);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^
>         not called in 8250 driver because UPF_SOFT_FLOW and UPF_HARD_FLOW are both unset
>>   Also call at the end
>>       uart_clear_mctrl(port, TIOCM_RTS);
>>   and this go an clear the MCR RTS bit.
>                    ^^^^^^^^^^^^^^^^^^^^^
>                    which turns off auto-RTS
>
> When uart_unthrottle() reenables MCR RTS, then auto-RTS kicks back on.
>
>
>> So what does uart_throttle() API expected to do since MCR is updated using uart_clear_mctrl().
>>
>> I searched who sets the UPF_HARD_FLOW in port->flags and only driver that does set this flag is omap-serial.c. The check was introduced by commit from Ruessel King to support h/w assisted h/w flow control.
>>
>> ======================================================================
>> commit dba05832cbe4f305dfd998fb26d7c685d91fbbd8
>> Author: Russell King<rmk+kernel@arm.linux.org.uk>
>> Date:   Tue Apr 17 16:41:10 2012 +0100
>>
>>      SERIAL: core: add hardware assisted h/w flow control support
>>
>>      Ports which are handling h/w flow control in hardware must not have
>>      their RTS state altered depending on the tty's hardware-stopped state.
>>      Avoid this additional logic when setting the termios state.
>>
>>      Acked-by: Alan Cox<alan@linux.intel.com>
>
>> ======================================================================
>>
>> This flag is checked in uart_set_termios() and the reason is in the commit log above.
>>
>> So shouldn't AFE support in 8250 make use of this flag?
>
> No. Hopefully my explanation of how the 8250 driver uses AFE clears that up.
>
>
>> My patch uses DT attribute to set this flag so above uart_set_termios() function behave as expected. But as you have rightly pointed out throttle()/unthrottle() is missing that needs to be added. Only driver that I can find that has implemented this is omap_serial.c. It does disable RDI and RLSI as part of throttle() interrupt and re-enable it as part of unthrottle(). Probably I can add this in this driver as well if this is what is expected. I will post a patch for this anyways, with some basic testing, but testing on customer h/w for this was initialially implemented has to wait until my return from vacation  (on vacation from 08/11-08/16).
>
> Which is why we should revert the previous patch, and restart the effort with
> a patch that adds a way to disable the fifo level check and see if that just
> works with your customer hardware (with UART_CAP_AFE set in the firmware).
>
>
>>>> Also 8250.c support other port types that doesn't have AFE. So shoudl this
>>>> be driving RTS line to stop the sender and resume when uart_unthrottle() is called?
>>>
>>> Yes, because that's how sw-assisted CTS flow control works, and the
>>> default behavior of uart_throttle/uart_unthrottle.
>>>
>>> IOW, with no extra code or special-casing, AFE just works.
>>>
>>
>> Based on my above discussion, there are few things required to be done on top of AFE and some of it is done by my patch and the remaining thing to be addressed in another patch.
>
> Assuming that AFE, as already implemented in the 8250 driver, works as expected,
> the fifo level check seems to be the only hurdle, right?

Also how uart_set_termios() expect to work without my patch? that is 
needed as well.

>
>
>>>> I want to work to fix this rather than revert this change as our customer is already using this feature.
>>>
>>> 3.16 was released 4 days ago.
>>
>> As I said, I will work to address this with priority.
>
> My point was that I'm not understanding how your customer could be using this
> feature when it came out 4 days ago, but yet now you can't even test on the
> hardware?

This fix was back ported to v3.13 that the customer is using.
>
> Regards,
> Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Peter Hurley Aug. 8, 2014, 10:09 p.m. UTC | #18
On 08/08/2014 05:02 PM, Murali Karicheri wrote:
> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>>
>> [...]
>>
>>>> But I realize now that a different question needs asking:
>>>> Is the MSR read showing delta CTS set when AFE is on, ever?
>>>
>>> Unfortunately this was tested on a customer board that I don't have access to and can't check this out right away. I am trying to findout if I can get some hardware to test the patch to address the issue being discussed. Customer board is currently using RTS and CTS lines and the same works fine for them with this patch.
>>
>> Ok.
>>
>>
>>>> Because serial8250_modem_status() assumes the answer is no for
>>>> _all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
>>>> is broken if AFE is on.
>>>
>>> As per Keystone UART spec
>>>
>>> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated
>>>
>>> So based on this, there shouldn't be any CTS change if AFE is enabled and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE() as you suggested to detect any offending h/w.
>>
>> That's identical wording to the 16750 datasheet.
>>
>> But notice that it only says "no interrupt is generated" when AFE is on.
>> It doesn't say if the MSR is read, that DCTS won't be set. And that's
>> an important difference for how serial8250_modem_status() works.
>>
>> [...]
>>
>>
>>>>>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>>>>>> for high-level rx flow control, such as when a read buffer fills up because
>>>>>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>>>>>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
>>>>>
>>>>> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell
>>>>> the remote from sending more bytes. So if h/w flow control is enabled, then not sure why
>>>>> uart_throttle() is to be doing anything when h/w flow control is supported? A dummy
>>>>> function required to satisfy the line discipline?
>>>>
>>>> I understand how auto-RTS works.
>>>>
>>>> Let's pretend for a moment that uart_throttle() does nothing when
>>>> auto-RTS is enabled:
>>>>
>>>> 1. tty buffers start to fill up because no process is reading the data.
>>>> 2. The throttle threshold is reached.
>>>> 3. uart_throttle() is called but does nothing.
>>>> 4. more data arrives and the DR interrupt is triggered
>>>> 5. serial8250_rx_chars() reads in the new data.
>>>> 6. tty buffers keep filling up even though the driver was told to throttle
>>>> 7. eventually the tty buffers will cap at about 64KB and start counting
>>>>      buf_overrun errors
>>>>
>>> Ok.
>>>
>>> Couple of observation on the AFE implementation in 8250 driver prior to my patch.
>>>
>>>  From the discussion so far, AFE is actually hardware assisted hardware flow control. Auto CTS is sw assisted hardware flow control
>>> where sw uses RTS line for recieve side flow control and I assume it uses MCR RTS bit for this where AFE does this automatically. From
>>> the 16550 or Keystone UART spec, I can't find how RTS line can be asserted to do this through sw instead of hardware doing it automatically. Spec says
>>>
>>> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
>>> e autoflow control enabled. Note that all UARTs do not support this feature. See the device-specific data manual for supported features. If this feature is not available, this bit is reserved and should be cleared to 0.
>>> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
>>> 1 = UARTn_RTS and UARTn_CTS are enabled.
>>>
>>> Then since AFE was already supported before my patch for FIFO size 32bytes or higher, I am wondering why there was no implementation of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not set at all if AFE implemented in 8250 driver is hw assisted, hw flow control. Also what do these API supposed to do?
>>
>> uart_throttle() does _not_ call ops->throttle() unless either
>> UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.
>>
> 
> Not based on port flag. Here is the actual code in serial_core.c as I see it.

You're misreading the code.


> static void uart_throttle(struct tty_struct *tty)
> {
>     struct uart_state *state = tty->driver_data;
>     struct uart_port *port = state->uart_port;
>     uint32_t mask = 0;
> 
>     if (I_IXOFF(tty))
>         mask |= UPF_SOFT_FLOW;
>     if (tty->termios.c_cflag & CRTSCTS)
>         mask |= UPF_HARD_FLOW;

mask = UPF_HARD_FLOW

port->flags does not have UPF_HARD_FLOW set so

         (port->flags & mask) == false

>     if (port->flags & mask) {
>         port->ops->throttle(port);
>         mask &= ~port->flags;
>     }
> 
>     if (mask & UPF_SOFT_FLOW)
>         uart_send_xchar(tty, STOP_CHAR(tty));
> 
>     if (mask & UPF_HARD_FLOW)
>         uart_clear_mctrl(port, TIOCM_RTS);
> }

[...]

>>> Based on my above discussion, there are few things required to be done on top of AFE and some of it is done by my patch and the remaining thing to be addressed in another patch.
>>
>> Assuming that AFE, as already implemented in the 8250 driver, works as expected,
>> the fifo level check seems to be the only hurdle, right?
> 
> Also how uart_set_termios() expect to work without my patch? that is needed as well.

That looks buggy, even if UPF_HARD_FLOW is set.

But that's my point: the most general cases should be fixed, if necessary.
Then, a trivial change to override the fifo size check from firmware is all you'll need


>>>>> I want to work to fix this rather than revert this change as our customer is already using this feature.
>>>>
>>>> 3.16 was released 4 days ago.
>>>
>>> As I said, I will work to address this with priority.
>>
>> My point was that I'm not understanding how your customer could be using this
>> feature when it came out 4 days ago, but yet now you can't even test on the
>> hardware?
> 
> This fix was back ported to v3.13 that the customer is using.

Ok, so your customer is running a custom kernel. Then I don't see the problem with backing
this change out, rather than building on top of it.

Regards,
Peter Hurley
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Murali Karicheri Aug. 8, 2014, 10:59 p.m. UTC | #19
On 08/08/2014 06:09 PM, Peter Hurley wrote:
> On 08/08/2014 05:02 PM, Murali Karicheri wrote:
>> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>>>
>>> [...]
>>>
>>>>> But I realize now that a different question needs asking:
>>>>> Is the MSR read showing delta CTS set when AFE is on, ever?
>>>>
>>>> Unfortunately this was tested on a customer board that I don't have access to and can't check this out right away. I am trying to findout if I can get some hardware to test the patch to address the issue being discussed. Customer board is currently using RTS and CTS lines and the same works fine for them with this patch.
>>>
>>> Ok.
>>>
>>>
>>>>> Because serial8250_modem_status() assumes the answer is no for
>>>>> _all_ AFE-capable devices, and if yes, would mean that serial8250_modem_status()
>>>>> is broken if AFE is on.
>>>>
>>>> As per Keystone UART spec
>>>>
>>>> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated
>>>>
>>>> So based on this, there shouldn't be any CTS change if AFE is enabled and will indicate change if AFE is disabled. Probably add WARN_ON_ONCE() as you suggested to detect any offending h/w.
>>>
>>> That's identical wording to the 16750 datasheet.
>>>
>>> But notice that it only says "no interrupt is generated" when AFE is on.
>>> It doesn't say if the MSR is read, that DCTS won't be set. And that's
>>> an important difference for how serial8250_modem_status() works.
>>>
>>> [...]
>>>
>>>
>>>>>>> uart_throttle() and uart_unthrottle() are used indirectly by line disciplines
>>>>>>> for high-level rx flow control, such as when a read buffer fills up because
>>>>>>> there is no userspace reader. The 8250 core doesn't define a throttle/unthrottle
>>>>>>> method because writing MCR to drop RTS is sufficient to disable auto-RTS.
>>>>>>
>>>>>> As per spec. hardware has rx threshold levels set to trigger an RTS level change to tell
>>>>>> the remote from sending more bytes. So if h/w flow control is enabled, then not sure why
>>>>>> uart_throttle() is to be doing anything when h/w flow control is supported? A dummy
>>>>>> function required to satisfy the line discipline?
>>>>>
>>>>> I understand how auto-RTS works.
>>>>>
>>>>> Let's pretend for a moment that uart_throttle() does nothing when
>>>>> auto-RTS is enabled:
>>>>>
>>>>> 1. tty buffers start to fill up because no process is reading the data.
>>>>> 2. The throttle threshold is reached.
>>>>> 3. uart_throttle() is called but does nothing.
>>>>> 4. more data arrives and the DR interrupt is triggered
>>>>> 5. serial8250_rx_chars() reads in the new data.
>>>>> 6. tty buffers keep filling up even though the driver was told to throttle
>>>>> 7. eventually the tty buffers will cap at about 64KB and start counting
>>>>>       buf_overrun errors
>>>>>
>>>> Ok.
>>>>
>>>> Couple of observation on the AFE implementation in 8250 driver prior to my patch.
>>>>
>>>>   From the discussion so far, AFE is actually hardware assisted hardware flow control. Auto CTS is sw assisted hardware flow control
>>>> where sw uses RTS line for recieve side flow control and I assume it uses MCR RTS bit for this where AFE does this automatically. From
>>>> the 16550 or Keystone UART spec, I can't find how RTS line can be asserted to do this through sw instead of hardware doing it automatically. Spec says
>>>>
>>>> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
>>>> e autoflow control enabled. Note that all UARTs do not support this feature. See the device-specific data manual for supported features. If this feature is not available, this bit is reserved and should be cleared to 0.
>>>> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
>>>> 1 = UARTn_RTS and UARTn_CTS are enabled.
>>>>
>>>> Then since AFE was already supported before my patch for FIFO size 32bytes or higher, I am wondering why there was no implementation of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag is not set at all if AFE implemented in 8250 driver is hw assisted, hw flow control. Also what do these API supposed to do?
>>>
>>> uart_throttle() does _not_ call ops->throttle() unless either
>>> UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.
>>>
>>
>> Not based on port flag. Here is the actual code in serial_core.c as I see it.
>
> You're misreading the code.
>
>
>> static void uart_throttle(struct tty_struct *tty)
>> {
>>      struct uart_state *state = tty->driver_data;
>>      struct uart_port *port = state->uart_port;
>>      uint32_t mask = 0;
>>
>>      if (I_IXOFF(tty))
>>          mask |= UPF_SOFT_FLOW;
>>      if (tty->termios.c_cflag&  CRTSCTS)
>>          mask |= UPF_HARD_FLOW;
>
> mask = UPF_HARD_FLOW
>
> port->flags does not have UPF_HARD_FLOW set so
>
>           (port->flags&  mask) == false
>

Ok. My bad.

>>      if (port->flags&  mask) {
>>          port->ops->throttle(port);
>>          mask&= ~port->flags;
>>      }
>>
>>      if (mask&  UPF_SOFT_FLOW)
>>          uart_send_xchar(tty, STOP_CHAR(tty));
>>
>>      if (mask&  UPF_HARD_FLOW)
>>          uart_clear_mctrl(port, TIOCM_RTS);
>> }
>
> [...]
>
>>>> Based on my above discussion, there are few things required to be done on top of AFE and some of it is done by my patch and the remaining thing to be addressed in another patch.
>>>
>>> Assuming that AFE, as already implemented in the 8250 driver, works as expected,
>>> the fifo level check seems to be the only hurdle, right?
>>
>> Also how uart_set_termios() expect to work without my patch? that is needed as well.
>
> That looks buggy, even if UPF_HARD_FLOW is set.
>
> But that's my point: the most general cases should be fixed, if necessary.
> Then, a trivial change to override the fifo size check from firmware is all you'll need


But then it seems like UPF_HARD_FLOW flag was introduced by 
dba05832cbe4f305dfd998fb26d7c685d91fbbd8 SERIAL: core: add hardware 
assisted h/w flow control support and I worked my patch around this. 
This is misleading.

Assume we don't use the UPF_HARD_FLOW anymore. Then in 
uart_set_termios(), how does it know if the port has hw assisted hw flow 
control? What is the other bug you mentioned?
>
>
>>>>>> I want to work to fix this rather than revert this change as our customer is already using this feature.
>>>>>
>>>>> 3.16 was released 4 days ago.
>>>>
>>>> As I said, I will work to address this with priority.
>>>
>>> My point was that I'm not understanding how your customer could be using this
>>> feature when it came out 4 days ago, but yet now you can't even test on the
>>> hardware?
>>
>> This fix was back ported to v3.13 that the customer is using.
>
> Ok, so your customer is running a custom kernel. Then I don't see the problem with backing
> this change out, rather than building on top of it.

Customer will soon be switching to newer kernel and this become an 
issue. So this must be addressed even if it requires a different fix.
At this point, I still think a fix is workable if we can make use of
existing UPF_HARD_FLOW flag that is meant for this scenario.

Assuming we re-use auto-flow-control instead of the has-hw-flow-control, 
and discard UPF_HARD_FLOW, we need to fix

1. limit to 32 bytes for fifo size as we have 16 bytes for keystone uart
2. uart_prt_startup() support for the hw flow control
3. uart_set_termios(), avoid stopping the hardware if port has hw flow
    control

For 1) no idea why 32 byte limit is required and for hw flow control 
this is not needed. For 2) and 3, how does the serial core driver knows 
if the uart port has the AFE capability with out using the flag.

I will restart this thread after my vacation. Meanwhile if you have 
suggestions as to how to deal with 1-3, please respond so that I can 
work on a patch based on it.

Thanks and regards,

Murali
>
> Regards,
> Peter Hurley
>
>

--
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
Murali Karicheri Aug. 9, 2014, 11:28 a.m. UTC | #20
On 08/08/2014 06:59 PM, Murali Karicheri wrote:
> On 08/08/2014 06:09 PM, Peter Hurley wrote:
>> On 08/08/2014 05:02 PM, Murali Karicheri wrote:
>>> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>>>>
>>>> [...]
>>>>
>>>>>> But I realize now that a different question needs asking:
>>>>>> Is the MSR read showing delta CTS set when AFE is on, ever?
>>>>>
>>>>> Unfortunately this was tested on a customer board that I don't have
>>>>> access to and can't check this out right away. I am trying to
>>>>> findout if I can get some hardware to test the patch to address the
>>>>> issue being discussed. Customer board is currently using RTS and
>>>>> CTS lines and the same works fine for them with this patch.
>>>>
>>>> Ok.
>>>>
>>>>
>>>>>> Because serial8250_modem_status() assumes the answer is no for
>>>>>> _all_ AFE-capable devices, and if yes, would mean that
>>>>>> serial8250_modem_status()
>>>>>> is broken if AFE is on.
>>>>>
>>>>> As per Keystone UART spec
>>>>>
>>>>> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates
>>>>> that the CTS input has changed state since the last time it was
>>>>> read by the CPU. When DCTS is set (autoflow control is not enabled
>>>>> and the modem status interrupt is enabled), a modem status
>>>>> interrupt is generated. When autoflow control is enabled, no
>>>>> interrupt is generated
>>>>>
>>>>> So based on this, there shouldn't be any CTS change if AFE is
>>>>> enabled and will indicate change if AFE is disabled. Probably add
>>>>> WARN_ON_ONCE() as you suggested to detect any offending h/w.
>>>>
>>>> That's identical wording to the 16750 datasheet.
>>>>
>>>> But notice that it only says "no interrupt is generated" when AFE is
>>>> on.
>>>> It doesn't say if the MSR is read, that DCTS won't be set. And that's
>>>> an important difference for how serial8250_modem_status() works.
>>>>
>>>> [...]
>>>>
>>>>
>>>>>>>> uart_throttle() and uart_unthrottle() are used indirectly by
>>>>>>>> line disciplines
>>>>>>>> for high-level rx flow control, such as when a read buffer fills
>>>>>>>> up because
>>>>>>>> there is no userspace reader. The 8250 core doesn't define a
>>>>>>>> throttle/unthrottle
>>>>>>>> method because writing MCR to drop RTS is sufficient to disable
>>>>>>>> auto-RTS.
>>>>>>>
>>>>>>> As per spec. hardware has rx threshold levels set to trigger an
>>>>>>> RTS level change to tell
>>>>>>> the remote from sending more bytes. So if h/w flow control is
>>>>>>> enabled, then not sure why
>>>>>>> uart_throttle() is to be doing anything when h/w flow control is
>>>>>>> supported? A dummy
>>>>>>> function required to satisfy the line discipline?
>>>>>>
>>>>>> I understand how auto-RTS works.
>>>>>>
>>>>>> Let's pretend for a moment that uart_throttle() does nothing when
>>>>>> auto-RTS is enabled:
>>>>>>
>>>>>> 1. tty buffers start to fill up because no process is reading the
>>>>>> data.
>>>>>> 2. The throttle threshold is reached.
>>>>>> 3. uart_throttle() is called but does nothing.
>>>>>> 4. more data arrives and the DR interrupt is triggered
>>>>>> 5. serial8250_rx_chars() reads in the new data.
>>>>>> 6. tty buffers keep filling up even though the driver was told to
>>>>>> throttle
>>>>>> 7. eventually the tty buffers will cap at about 64KB and start
>>>>>> counting
>>>>>> buf_overrun errors
>>>>>>
>>>>> Ok.
>>>>>
>>>>> Couple of observation on the AFE implementation in 8250 driver
>>>>> prior to my patch.
>>>>>
>>>>> From the discussion so far, AFE is actually hardware assisted
>>>>> hardware flow control. Auto CTS is sw assisted hardware flow control
>>>>> where sw uses RTS line for recieve side flow control and I assume
>>>>> it uses MCR RTS bit for this where AFE does this automatically. From
>>>>> the 16550 or Keystone UART spec, I can't find how RTS line can be
>>>>> asserted to do this through sw instead of hardware doing it
>>>>> automatically. Spec says
>>>>>
>>>>> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
>>>>> e autoflow control enabled. Note that all UARTs do not support this
>>>>> feature. See the device-specific data manual for supported
>>>>> features. If this feature is not available, this bit is reserved
>>>>> and should be cleared to 0.
>>>>> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
>>>>> 1 = UARTn_RTS and UARTn_CTS are enabled.
>>>>>
>>>>> Then since AFE was already supported before my patch for FIFO size
>>>>> 32bytes or higher, I am wondering why there was no implementation
>>>>> of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag
>>>>> is not set at all if AFE implemented in 8250 driver is hw assisted,
>>>>> hw flow control. Also what do these API supposed to do?
>>>>
>>>> uart_throttle() does _not_ call ops->throttle() unless either
>>>> UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.
>>>>
>>>
>>> Not based on port flag. Here is the actual code in serial_core.c as I
>>> see it.
>>
>> You're misreading the code.
>>
>>
>>> static void uart_throttle(struct tty_struct *tty)
>>> {
>>> struct uart_state *state = tty->driver_data;
>>> struct uart_port *port = state->uart_port;
>>> uint32_t mask = 0;
>>>
>>> if (I_IXOFF(tty))
>>> mask |= UPF_SOFT_FLOW;
>>> if (tty->termios.c_cflag& CRTSCTS)
>>> mask |= UPF_HARD_FLOW;
>>
>> mask = UPF_HARD_FLOW
>>
>> port->flags does not have UPF_HARD_FLOW set so
>>
>> (port->flags& mask) == false
>>
>
> Ok. My bad.
>
>>> if (port->flags& mask) {
>>> port->ops->throttle(port);
>>> mask&= ~port->flags;
>>> }
>>>
>>> if (mask& UPF_SOFT_FLOW)
>>> uart_send_xchar(tty, STOP_CHAR(tty));
>>>
>>> if (mask& UPF_HARD_FLOW)
>>> uart_clear_mctrl(port, TIOCM_RTS);
>>> }
>>
>> [...]
>>
>>>>> Based on my above discussion, there are few things required to be
>>>>> done on top of AFE and some of it is done by my patch and the
>>>>> remaining thing to be addressed in another patch.
>>>>
>>>> Assuming that AFE, as already implemented in the 8250 driver, works
>>>> as expected,
>>>> the fifo level check seems to be the only hurdle, right?
>>>
>>> Also how uart_set_termios() expect to work without my patch? that is
>>> needed as well.
>>
>> That looks buggy, even if UPF_HARD_FLOW is set.
>>
>> But that's my point: the most general cases should be fixed, if
>> necessary.
>> Then, a trivial change to override the fifo size check from firmware
>> is all you'll need
>
>
> But then it seems like UPF_HARD_FLOW flag was introduced by
> dba05832cbe4f305dfd998fb26d7c685d91fbbd8 SERIAL: core: add hardware
> assisted h/w flow control support and I worked my patch around this.
> This is misleading.
>
> Assume we don't use the UPF_HARD_FLOW anymore. Then in
> uart_set_termios(), how does it know if the port has hw assisted hw flow
> control? What is the other bug you mentioned?
>>
>>
>>>>>>> I want to work to fix this rather than revert this change as our
>>>>>>> customer is already using this feature.
>>>>>>
>>>>>> 3.16 was released 4 days ago.
>>>>>
>>>>> As I said, I will work to address this with priority.
>>>>
>>>> My point was that I'm not understanding how your customer could be
>>>> using this
>>>> feature when it came out 4 days ago, but yet now you can't even test
>>>> on the
>>>> hardware?
>>>
>>> This fix was back ported to v3.13 that the customer is using.
>>
>> Ok, so your customer is running a custom kernel. Then I don't see the
>> problem with backing
>> this change out, rather than building on top of it.
>
> Customer will soon be switching to newer kernel and this become an
> issue. So this must be addressed even if it requires a different fix.
> At this point, I still think a fix is workable if we can make use of
> existing UPF_HARD_FLOW flag that is meant for this scenario.
>
> Assuming we re-use auto-flow-control instead of the has-hw-flow-control,
> and discard UPF_HARD_FLOW, we need to fix
>
> 1. limit to 32 bytes for fifo size as we have 16 bytes for keystone uart
> 2. uart_prt_startup() support for the hw flow control
> 3. uart_set_termios(), avoid stopping the hardware if port has hw flow
> control
>
> For 1) no idea why 32 byte limit is required and for hw flow control
> this is not needed. For 2) and 3, how does the serial core driver knows
> if the uart port has the AFE capability with out using the flag.
>

Peter,

I want to add one more piece of information related to my original patch 
that I had forgotten to mention so that right decision can be taken on this.

The patch was added for one more use case with a different customer. The 
use case was running BT over uart and this required hw flow control. In 
their testing they have never encountered any issue w.r.t throttle when 
they had run their performance test. So it makes me believe throttle is 
in fact may not be needed for keystone UART wih h/w flow control. So we 
might as well add a check in serial-core.c to check if 
throttle()/unthrottle() is implemented and then invoke it. This should 
address your concern. Also in your description of AFE, default behavior 
is good enough for AFE.

Regarding the second issue, the change was added for the BT use case. As 
I don't have access to this customer's hardware, I wouldn't be able to 
verify if this use case indeed causes call to uart_handle_cts_change() 
due to a hardware bug since as per spec below, it is not supposed to 
generate interrupt or CTS change.

DCTS - Change in CTS indicator bit. DCTS indicates that the CTS input 
has changed state since the last time it was read by the CPU. When DCTS 
is set (autoflow control is not enabled and the modem status interrupt 
is  enabled), a modem status interrupt is generated. When autoflow 
control is enabled, no interrupt is generated.

I believe this check indeed can be moved to the 8250 function that make 
call to this and also increment the cts count as done in this function 
so that we could verify if this indeed increases for the AFE casee. I 
might be able to query the customer for the CTS count ever increase with 
BT use case, then if it doesn't this may be removed later or keep it to 
address the hardware issue.

As this patch was added to support 2 different use cases, one for a 
virtual serial port and another for BT over uart, I would strongly defer 
from reverting this patch and add a fix as described above. Do you know 
if there is any bug report because of this commit or you raised it as 
part of reviewing the code? If latter, I could send out a patch to fix 
it as described above.

Hope this will not get reverted and I will have an opportunity to send a 
fix once I am back from my vacation.

Thanks and regards,

Murali

> I will restart this thread after my vacation. Meanwhile if you have
> suggestions as to how to deal with 1-3, please respond so that I can
> work on a patch based on it.
>
> Thanks and regards,
>
> Murali
>>
>> Regards,
>> Peter Hurley
>>
>>
>

--
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
Murali Karicheri Aug. 19, 2014, 3:29 p.m. UTC | #21
On 08/09/2014 07:28 AM, Murali Karicheri wrote:
> On 08/08/2014 06:59 PM, Murali Karicheri wrote:
>> On 08/08/2014 06:09 PM, Peter Hurley wrote:
>>> On 08/08/2014 05:02 PM, Murali Karicheri wrote:
>>>> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>>>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>>>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>> But I realize now that a different question needs asking:
>>>>>>> Is the MSR read showing delta CTS set when AFE is on, ever?
>>>>>>
>>>>>> Unfortunately this was tested on a customer board that I don't have
>>>>>> access to and can't check this out right away. I am trying to
>>>>>> findout if I can get some hardware to test the patch to address the
>>>>>> issue being discussed. Customer board is currently using RTS and
>>>>>> CTS lines and the same works fine for them with this patch.
>>>>>
>>>>> Ok.
>>>>>
>>>>>
>>>>>>> Because serial8250_modem_status() assumes the answer is no for
>>>>>>> _all_ AFE-capable devices, and if yes, would mean that
>>>>>>> serial8250_modem_status()
>>>>>>> is broken if AFE is on.
>>>>>>
>>>>>> As per Keystone UART spec
>>>>>>
>>>>>> bit 0 in MSR: DCTS: Change in CTS indicator bit. DCTS indicates
>>>>>> that the CTS input has changed state since the last time it was
>>>>>> read by the CPU. When DCTS is set (autoflow control is not enabled
>>>>>> and the modem status interrupt is enabled), a modem status
>>>>>> interrupt is generated. When autoflow control is enabled, no
>>>>>> interrupt is generated
>>>>>>
>>>>>> So based on this, there shouldn't be any CTS change if AFE is
>>>>>> enabled and will indicate change if AFE is disabled. Probably add
>>>>>> WARN_ON_ONCE() as you suggested to detect any offending h/w.
>>>>>
>>>>> That's identical wording to the 16750 datasheet.
>>>>>
>>>>> But notice that it only says "no interrupt is generated" when AFE is
>>>>> on.
>>>>> It doesn't say if the MSR is read, that DCTS won't be set. And that's
>>>>> an important difference for how serial8250_modem_status() works.
>>>>>
>>>>> [...]
>>>>>
>>>>>
>>>>>>>>> uart_throttle() and uart_unthrottle() are used indirectly by
>>>>>>>>> line disciplines
>>>>>>>>> for high-level rx flow control, such as when a read buffer fills
>>>>>>>>> up because
>>>>>>>>> there is no userspace reader. The 8250 core doesn't define a
>>>>>>>>> throttle/unthrottle
>>>>>>>>> method because writing MCR to drop RTS is sufficient to disable
>>>>>>>>> auto-RTS.
>>>>>>>>
>>>>>>>> As per spec. hardware has rx threshold levels set to trigger an
>>>>>>>> RTS level change to tell
>>>>>>>> the remote from sending more bytes. So if h/w flow control is
>>>>>>>> enabled, then not sure why
>>>>>>>> uart_throttle() is to be doing anything when h/w flow control is
>>>>>>>> supported? A dummy
>>>>>>>> function required to satisfy the line discipline?
>>>>>>>
>>>>>>> I understand how auto-RTS works.
>>>>>>>
>>>>>>> Let's pretend for a moment that uart_throttle() does nothing when
>>>>>>> auto-RTS is enabled:
>>>>>>>
>>>>>>> 1. tty buffers start to fill up because no process is reading the
>>>>>>> data.
>>>>>>> 2. The throttle threshold is reached.
>>>>>>> 3. uart_throttle() is called but does nothing.
>>>>>>> 4. more data arrives and the DR interrupt is triggered
>>>>>>> 5. serial8250_rx_chars() reads in the new data.
>>>>>>> 6. tty buffers keep filling up even though the driver was told to
>>>>>>> throttle
>>>>>>> 7. eventually the tty buffers will cap at about 64KB and start
>>>>>>> counting
>>>>>>> buf_overrun errors
>>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Couple of observation on the AFE implementation in 8250 driver
>>>>>> prior to my patch.
>>>>>>
>>>>>> From the discussion so far, AFE is actually hardware assisted
>>>>>> hardware flow control. Auto CTS is sw assisted hardware flow control
>>>>>> where sw uses RTS line for recieve side flow control and I assume
>>>>>> it uses MCR RTS bit for this where AFE does this automatically. From
>>>>>> the 16550 or Keystone UART spec, I can't find how RTS line can be
>>>>>> asserted to do this through sw instead of hardware doing it
>>>>>> automatically. Spec says
>>>>>>
>>>>>> MCR RTS bit: RTS control. When AFE = 1, the RTS bit determines th
>>>>>> e autoflow control enabled. Note that all UARTs do not support this
>>>>>> feature. See the device-specific data manual for supported
>>>>>> features. If this feature is not available, this bit is reserved
>>>>>> and should be cleared to 0.
>>>>>> 0 = UARTn_RTS is disabled, only UARTn_CTS is enabled.
>>>>>> 1 = UARTn_RTS and UARTn_CTS are enabled.
>>>>>>
>>>>>> Then since AFE was already supported before my patch for FIFO size
>>>>>> 32bytes or higher, I am wondering why there was no implementation
>>>>>> of throttle()/unthrottle() to begin with and why UPF_HARD_FLOW flag
>>>>>> is not set at all if AFE implemented in 8250 driver is hw assisted,
>>>>>> hw flow control. Also what do these API supposed to do?
>>>>>
>>>>> uart_throttle() does _not_ call ops->throttle() unless either
>>>>> UPF_SOFT_FLOW and/or UPF_HARD_FLOW is set in uport->flags.
>>>>>
>>>>
>>>> Not based on port flag. Here is the actual code in serial_core.c as I
>>>> see it.
>>>
>>> You're misreading the code.
>>>
>>>
>>>> static void uart_throttle(struct tty_struct *tty)
>>>> {
>>>> struct uart_state *state = tty->driver_data;
>>>> struct uart_port *port = state->uart_port;
>>>> uint32_t mask = 0;
>>>>
>>>> if (I_IXOFF(tty))
>>>> mask |= UPF_SOFT_FLOW;
>>>> if (tty->termios.c_cflag& CRTSCTS)
>>>> mask |= UPF_HARD_FLOW;
>>>
>>> mask = UPF_HARD_FLOW
>>>
>>> port->flags does not have UPF_HARD_FLOW set so
>>>
>>> (port->flags& mask) == false
>>>
>>
>> Ok. My bad.
>>
>>>> if (port->flags& mask) {
>>>> port->ops->throttle(port);
>>>> mask&= ~port->flags;
>>>> }
>>>>
>>>> if (mask& UPF_SOFT_FLOW)
>>>> uart_send_xchar(tty, STOP_CHAR(tty));
>>>>
>>>> if (mask& UPF_HARD_FLOW)
>>>> uart_clear_mctrl(port, TIOCM_RTS);
>>>> }
>>>
>>> [...]
>>>
>>>>>> Based on my above discussion, there are few things required to be
>>>>>> done on top of AFE and some of it is done by my patch and the
>>>>>> remaining thing to be addressed in another patch.
>>>>>
>>>>> Assuming that AFE, as already implemented in the 8250 driver, works
>>>>> as expected,
>>>>> the fifo level check seems to be the only hurdle, right?
>>>>
>>>> Also how uart_set_termios() expect to work without my patch? that is
>>>> needed as well.
>>>
>>> That looks buggy, even if UPF_HARD_FLOW is set.
>>>
>>> But that's my point: the most general cases should be fixed, if
>>> necessary.
>>> Then, a trivial change to override the fifo size check from firmware
>>> is all you'll need
>>
>>
>> But then it seems like UPF_HARD_FLOW flag was introduced by
>> dba05832cbe4f305dfd998fb26d7c685d91fbbd8 SERIAL: core: add hardware
>> assisted h/w flow control support and I worked my patch around this.
>> This is misleading.
>>
>> Assume we don't use the UPF_HARD_FLOW anymore. Then in
>> uart_set_termios(), how does it know if the port has hw assisted hw flow
>> control? What is the other bug you mentioned?
>>>
>>>
>>>>>>>> I want to work to fix this rather than revert this change as our
>>>>>>>> customer is already using this feature.
>>>>>>>
>>>>>>> 3.16 was released 4 days ago.
>>>>>>
>>>>>> As I said, I will work to address this with priority.
>>>>>
>>>>> My point was that I'm not understanding how your customer could be
>>>>> using this
>>>>> feature when it came out 4 days ago, but yet now you can't even test
>>>>> on the
>>>>> hardware?
>>>>
>>>> This fix was back ported to v3.13 that the customer is using.
>>>
>>> Ok, so your customer is running a custom kernel. Then I don't see the
>>> problem with backing
>>> this change out, rather than building on top of it.
>>
>> Customer will soon be switching to newer kernel and this become an
>> issue. So this must be addressed even if it requires a different fix.
>> At this point, I still think a fix is workable if we can make use of
>> existing UPF_HARD_FLOW flag that is meant for this scenario.
>>
>> Assuming we re-use auto-flow-control instead of the has-hw-flow-control,
>> and discard UPF_HARD_FLOW, we need to fix
>>
>> 1. limit to 32 bytes for fifo size as we have 16 bytes for keystone uart
>> 2. uart_prt_startup() support for the hw flow control
>> 3. uart_set_termios(), avoid stopping the hardware if port has hw flow
>> control
>>
>> For 1) no idea why 32 byte limit is required and for hw flow control
>> this is not needed. For 2) and 3, how does the serial core driver knows
>> if the uart port has the AFE capability with out using the flag.
>>
>
> Peter,
>
> I want to add one more piece of information related to my original patch
> that I had forgotten to mention so that right decision can be taken on
> this.
>
> The patch was added for one more use case with a different customer. The
> use case was running BT over uart and this required hw flow control. In
> their testing they have never encountered any issue w.r.t throttle when
> they had run their performance test. So it makes me believe throttle is
> in fact may not be needed for keystone UART wih h/w flow control. So we
> might as well add a check in serial-core.c to check if
> throttle()/unthrottle() is implemented and then invoke it. This should
> address your concern. Also in your description of AFE, default behavior
> is good enough for AFE.
>
> Regarding the second issue, the change was added for the BT use case. As
> I don't have access to this customer's hardware, I wouldn't be able to
> verify if this use case indeed causes call to uart_handle_cts_change()
> due to a hardware bug since as per spec below, it is not supposed to
> generate interrupt or CTS change.
>
> DCTS - Change in CTS indicator bit. DCTS indicates that the CTS input
> has changed state since the last time it was read by the CPU. When DCTS
> is set (autoflow control is not enabled and the modem status interrupt
> is enabled), a modem status interrupt is generated. When autoflow
> control is enabled, no interrupt is generated.
>
> I believe this check indeed can be moved to the 8250 function that make
> call to this and also increment the cts count as done in this function
> so that we could verify if this indeed increases for the AFE casee. I
> might be able to query the customer for the CTS count ever increase with
> BT use case, then if it doesn't this may be removed later or keep it to
> address the hardware issue.
>
> As this patch was added to support 2 different use cases, one for a
> virtual serial port and another for BT over uart, I would strongly defer
> from reverting this patch and add a fix as described above. Do you know
> if there is any bug report because of this commit or you raised it as
> part of reviewing the code? If latter, I could send out a patch to fix
> it as described above.
>
> Hope this will not get reverted and I will have an opportunity to send a
> fix once I am back from my vacation.
>
> Thanks and regards,
>
> Murali
>
>> I will restart this thread after my vacation. Meanwhile if you have
>> suggestions as to how to deal with 1-3, please respond so that I can
>> work on a patch based on it.
>>
>> Thanks and regards,
>>
>> Murali
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>
>>
>
Peter,

I am back from vacation and want to continue this thread until we agree 
on a solution to this issue. Please review my last few emails and let me 
know your response.

Murali
--
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
Peter Hurley Aug. 21, 2014, 7:33 p.m. UTC | #22
On 08/09/2014 07:28 AM, Murali Karicheri wrote:
> On 08/08/2014 06:59 PM, Murali Karicheri wrote:
>> On 08/08/2014 06:09 PM, Peter Hurley wrote:
>>> On 08/08/2014 05:02 PM, Murali Karicheri wrote:
>>>> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>>>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>>>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:

[cut]

>>>>>> Based on my above discussion, there are few things required to be
>>>>>> done on top of AFE and some of it is done by my patch and the
>>>>>> remaining thing to be addressed in another patch.
>>>>>
>>>>> Assuming that AFE, as already implemented in the 8250 driver, works
>>>>> as expected,
>>>>> the fifo level check seems to be the only hurdle, right?
>>>>
>>>> Also how uart_set_termios() expect to work without my patch? that is
>>>> needed as well.
>>>
>>> That looks buggy, even if UPF_HARD_FLOW is set.
>>>
>>> But that's my point: the most general cases should be fixed, if
>>> necessary.
>>> Then, a trivial change to override the fifo size check from firmware
>>> is all you'll need
>>
>>
>> But then it seems like UPF_HARD_FLOW flag was introduced by
>> dba05832cbe4f305dfd998fb26d7c685d91fbbd8 SERIAL: core: add hardware
>> assisted h/w flow control support and I worked my patch around this.
>> This is misleading.
>>
>> Assume we don't use the UPF_HARD_FLOW anymore. Then in
>> uart_set_termios(), how does it know if the port has hw assisted hw flow
>> control?

Well UPF_HARD_FLOW seems to have been added specifically to handle
the requirements of the omap UART driver, rather than a comprehensive
API for handling hw flow control.

>> What is the other bug you mentioned?

It assumes the port hasn't been reconfigured from non-UPF_HARD_FLOW to
UPF_HARD_FLOW.

>>>>>>>> I want to work to fix this rather than revert this change as our
>>>>>>>> customer is already using this feature.
>>>>>>>
>>>>>>> 3.16 was released 4 days ago.
>>>>>>
>>>>>> As I said, I will work to address this with priority.
>>>>>
>>>>> My point was that I'm not understanding how your customer could be
>>>>> using this
>>>>> feature when it came out 4 days ago, but yet now you can't even test
>>>>> on the
>>>>> hardware?
>>>>
>>>> This fix was back ported to v3.13 that the customer is using.
>>>
>>> Ok, so your customer is running a custom kernel. Then I don't see the
>>> problem with backing
>>> this change out, rather than building on top of it.
>>
>> Customer will soon be switching to newer kernel and this become an
>> issue. So this must be addressed even if it requires a different fix.
>> At this point, I still think a fix is workable if we can make use of
>> existing UPF_HARD_FLOW flag that is meant for this scenario.
>>
>> Assuming we re-use auto-flow-control instead of the has-hw-flow-control,
>> and discard UPF_HARD_FLOW, we need to fix
>>
>> 1. limit to 32 bytes for fifo size as we have 16 bytes for keystone uart
>> 2. uart_prt_startup() support for the hw flow control
>> 3. uart_set_termios(), avoid stopping the hardware if port has hw flow
>> control
>>
>> For 1) no idea why 32 byte limit is required and for hw flow control
>> this is not needed. For 2) and 3, how does the serial core driver knows
>> if the uart port has the AFE capability with out using the flag.

As long as either UART_IER_RLSI or UART_IER_RDI is _always_ enabled, the
fifo size test is bogus; received data will continue to be read so the
receive FIFO will never overflow.

However, if throttling involves disabling both interrupts (UART_IER_RLSI
and UART_IER_RDI) _and_ the remote is not using hw flow control, then
the receive FIFO could overflow (because the remote has not stopped
transmitting fast enough and the 8250 irq handler is not reading data).

Since the 8250 driver does not disable either the line status or data
ready interrupt, throttling does not cause rx fifo overflow. It's
possible that buggy auto RTS implementations stop generating those
interrupts when RTS is deasserted automatically, but that should be
quirked for that silicon only if discovered to be true.

> I want to add one more piece of information related to my original patch that I had forgotten to mention so that right decision can be taken on this.
> 
> The patch was added for one more use case with a different customer. The use case was running BT over uart and this required hw flow control. In their testing they have never encountered any issue w.r.t throttle when they had run their performance test. So it makes me believe throttle is in fact may not be needed for keystone UART wih h/w flow control. So we might as well add a check in serial-core.c to check if throttle()/unthrottle() is implemented and then invoke it. This should address your concern. Also in your description of AFE, default behavior is good enough for AFE.

The bluetooth line discipline doesn't currently throttle; but if this is ever
fixed, the UART driver will start mysteriously blowing up because it didn't
implement throttle/unthrottle methods.

> Regarding the second issue, the change was added for the BT use case. As I don't have access to this customer's hardware, I wouldn't be able to verify if this use case indeed causes call to uart_handle_cts_change() due to a hardware bug since as per spec below, it is not supposed to generate interrupt or CTS change.
> 
> DCTS - Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is  enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated.

I think DCTS is only latching the CTS input and has nothing to do with whether
an interrupt is being generated. IOW, a data ready interrupt will cause
the MSR to be read (in serial8250_modem_status()) which may indicate DCTS set,
if CTS was dropped to throttle this sender. Then if, for example, CLOCAL is off
and thus DCD is being monitored, this will cause uart_handle_cts_change() to be
called (because UART_IER_MSI will be on).

> I believe this check indeed can be moved to the 8250 function that make call to this and also increment the cts count as done in this function so that we could verify if this indeed increases for the AFE casee. I might be able to query the customer for the CTS count ever increase with BT use case, then if it doesn't this may be removed later or keep it to address the hardware issue.
> 
> As this patch was added to support 2 different use cases, one for a virtual serial port and another for BT over uart, I would strongly defer from reverting this patch and add a fix as described above. Do you know if there is any bug report because of this commit or you raised it as part of reviewing the code? If latter, I could send out a patch to fix it as described above.
> 
> Hope this will not get reverted and I will have an opportunity to send a fix once I am back from my vacation.

I think the way forward is:

1. Revert your patch
2. Remove the fifo size check

At this point both your use cases should work properly as the extra CTS
handling is just overhead and won't actually cause misbehavior.

Then, if you want to move forward from that point by eliminating the
extra CTS handling overhead then (as I see it) what's required is:

1. UPF_AUTO_CTS flag set if UART_EFR_CTS or UART_MCR_AFE (8250-specific)
2. Add UPF_AUTO_CTS test to UART_ENABLE_MS()
3. Suppress calls to uart_handle_cts_change if UPF_AUTO_CTS (or simply return
   from uart_handle_cts_change())
4. Set UPF_AUTO_CTS if UPF_HARD_FLOW is set (iow, UPF_HARD_FLOW should imply
   UPF_AUTO_CTS)
5. Check UPF_AUTO_CTS in uart_set_termios() (instead of UPF_HARD_FLOW),
   but only when changing to CRTSCTS, and not from CRTSCTS.
6. Override the check in uart_port_startup() if UPF_AUTO_CTS.

Other drivers that support auto CTS would also set UPF_AUTO_CTS; eg.,
mxs_auart and bfin_uart (when CONFIG_SERIAL_BFIN_HARD_CTSRTS).

UPF_HARD_FLOW should be scrapped, maybe along with the UART driver
throttle/unthrottle methods (although I haven't really given any thought
to handling in-band auto flow, aka UPF_SOFT_FLOW).

Any driver doing auto RTS would then override RTS by whatever method
required _whenever_ its set_mctrl() method indicates RTS has been lowered
(and reenabled when RTS is raised).

Right now, a driver doing UPF_HARD_FLOW via throttle/unthrottle doesn't
behave properly when the TIOCMSET ioctl is used to manually flow control
from userspace.

A UPF_AUTO_RTS flag could be added to help the UART driver remember
that its set_mctrl() method needs to virtualize RTS if required, like
for uarts where MCR RTS = 0 does not override the auto RTS setting.

Lastly, if a UART driver _knows_ that the remote is also wired for auto RTS
(like via a firmware flag), then its set_mctrl() method could _also_ disable
line status and data ready interrupts when RTS is lowered. UARTs with
a big rx fifo could do this always.

I would have already done this but I'm knee-deep in large patchsets for
serial and tty already.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri Aug. 22, 2014, 4:45 p.m. UTC | #23
On 08/21/2014 03:33 PM, Peter Hurley wrote:
> On 08/09/2014 07:28 AM, Murali Karicheri wrote:
>> On 08/08/2014 06:59 PM, Murali Karicheri wrote:
>>> On 08/08/2014 06:09 PM, Peter Hurley wrote:
>>>> On 08/08/2014 05:02 PM, Murali Karicheri wrote:
>>>>> On 08/08/2014 04:44 PM, Peter Hurley wrote:
>>>>>> On 08/08/2014 03:36 PM, Murali Karicheri wrote:
>>>>>>> On 08/07/2014 07:03 PM, Peter Hurley wrote:
>
> [cut]
>
>>>>>>> Based on my above discussion, there are few things required to be
>>>>>>> done on top of AFE and some of it is done by my patch and the
>>>>>>> remaining thing to be addressed in another patch.
>>>>>>
>>>>>> Assuming that AFE, as already implemented in the 8250 driver, works
>>>>>> as expected,
>>>>>> the fifo level check seems to be the only hurdle, right?
>>>>>
>>>>> Also how uart_set_termios() expect to work without my patch? that is
>>>>> needed as well.
>>>>
>>>> That looks buggy, even if UPF_HARD_FLOW is set.
>>>>
>>>> But that's my point: the most general cases should be fixed, if
>>>> necessary.
>>>> Then, a trivial change to override the fifo size check from firmware
>>>> is all you'll need
>>>
>>>
>>> But then it seems like UPF_HARD_FLOW flag was introduced by
>>> dba05832cbe4f305dfd998fb26d7c685d91fbbd8 SERIAL: core: add hardware
>>> assisted h/w flow control support and I worked my patch around this.
>>> This is misleading.
>>>
>>> Assume we don't use the UPF_HARD_FLOW anymore. Then in
>>> uart_set_termios(), how does it know if the port has hw assisted hw flow
>>> control?
>
> Well UPF_HARD_FLOW seems to have been added specifically to handle
> the requirements of the omap UART driver, rather than a comprehensive
> API for handling hw flow control.
>
>>> What is the other bug you mentioned?
>
> It assumes the port hasn't been reconfigured from non-UPF_HARD_FLOW to
> UPF_HARD_FLOW.
>
>>>>>>>>> I want to work to fix this rather than revert this change as our
>>>>>>>>> customer is already using this feature.
>>>>>>>>
>>>>>>>> 3.16 was released 4 days ago.
>>>>>>>
>>>>>>> As I said, I will work to address this with priority.
>>>>>>
>>>>>> My point was that I'm not understanding how your customer could be
>>>>>> using this
>>>>>> feature when it came out 4 days ago, but yet now you can't even test
>>>>>> on the
>>>>>> hardware?
>>>>>
>>>>> This fix was back ported to v3.13 that the customer is using.
>>>>
>>>> Ok, so your customer is running a custom kernel. Then I don't see the
>>>> problem with backing
>>>> this change out, rather than building on top of it.
>>>
>>> Customer will soon be switching to newer kernel and this become an
>>> issue. So this must be addressed even if it requires a different fix.
>>> At this point, I still think a fix is workable if we can make use of
>>> existing UPF_HARD_FLOW flag that is meant for this scenario.
>>>
>>> Assuming we re-use auto-flow-control instead of the has-hw-flow-control,
>>> and discard UPF_HARD_FLOW, we need to fix
>>>
>>> 1. limit to 32 bytes for fifo size as we have 16 bytes for keystone uart
>>> 2. uart_prt_startup() support for the hw flow control
>>> 3. uart_set_termios(), avoid stopping the hardware if port has hw flow
>>> control
>>>
>>> For 1) no idea why 32 byte limit is required and for hw flow control
>>> this is not needed. For 2) and 3, how does the serial core driver knows
>>> if the uart port has the AFE capability with out using the flag.
>
> As long as either UART_IER_RLSI or UART_IER_RDI is _always_ enabled, the
> fifo size test is bogus; received data will continue to be read so the
> receive FIFO will never overflow.
>
> However, if throttling involves disabling both interrupts (UART_IER_RLSI
> and UART_IER_RDI) _and_ the remote is not using hw flow control, then
> the receive FIFO could overflow (because the remote has not stopped
> transmitting fast enough and the 8250 irq handler is not reading data).
>
> Since the 8250 driver does not disable either the line status or data
> ready interrupt, throttling does not cause rx fifo overflow. It's
> possible that buggy auto RTS implementations stop generating those
> interrupts when RTS is deasserted automatically, but that should be
> quirked for that silicon only if discovered to be true.
>
>> I want to add one more piece of information related to my original patch that I had forgotten to mention so that right decision can be taken on this.
>>
>> The patch was added for one more use case with a different customer. The use case was running BT over uart and this required hw flow control. In their testing they have never encountered any issue w.r.t throttle when they had run their performance test. So it makes me believe throttle is in fact may not be needed for keystone UART wih h/w flow control. So we might as well add a check in serial-core.c to check if throttle()/unthrottle() is implemented and then invoke it. This should address your concern. Also in your description of AFE, default behavior is good enough for AFE.
>
> The bluetooth line discipline doesn't currently throttle; but if this is ever
> fixed, the UART driver will start mysteriously blowing up because it didn't
> implement throttle/unthrottle methods.
>
>> Regarding the second issue, the change was added for the BT use case. As I don't have access to this customer's hardware, I wouldn't be able to verify if this use case indeed causes call to uart_handle_cts_change() due to a hardware bug since as per spec below, it is not supposed to generate interrupt or CTS change.
>>
>> DCTS - Change in CTS indicator bit. DCTS indicates that the CTS input has changed state since the last time it was read by the CPU. When DCTS is set (autoflow control is not enabled and the modem status interrupt is  enabled), a modem status interrupt is generated. When autoflow control is enabled, no interrupt is generated.
>
> I think DCTS is only latching the CTS input and has nothing to do with whether
> an interrupt is being generated. IOW, a data ready interrupt will cause
> the MSR to be read (in serial8250_modem_status()) which may indicate DCTS set,
> if CTS was dropped to throttle this sender. Then if, for example, CLOCAL is off
> and thus DCD is being monitored, this will cause uart_handle_cts_change() to be
> called (because UART_IER_MSI will be on).
>
>> I believe this check indeed can be moved to the 8250 function that make call to this and also increment the cts count as done in this function so that we could verify if this indeed increases for the AFE casee. I might be able to query the customer for the CTS count ever increase with BT use case, then if it doesn't this may be removed later or keep it to address the hardware issue.
>>
>> As this patch was added to support 2 different use cases, one for a virtual serial port and another for BT over uart, I would strongly defer from reverting this patch and add a fix as described above. Do you know if there is any bug report because of this commit or you raised it as part of reviewing the code? If latter, I could send out a patch to fix it as described above.
>>
>> Hope this will not get reverted and I will have an opportunity to send a fix once I am back from my vacation.
>
> I think the way forward is:
>
> 1. Revert your patch
> 2. Remove the fifo size check
>
> At this point both your use cases should work properly as the extra CTS
> handling is just overhead and won't actually cause misbehavior.
>
> Then, if you want to move forward from that point by eliminating the
> extra CTS handling overhead then (as I see it) what's required is:
>
> 1. UPF_AUTO_CTS flag set if UART_EFR_CTS or UART_MCR_AFE (8250-specific)
> 2. Add UPF_AUTO_CTS test to UART_ENABLE_MS()
> 3. Suppress calls to uart_handle_cts_change if UPF_AUTO_CTS (or simply return
>     from uart_handle_cts_change())
> 4. Set UPF_AUTO_CTS if UPF_HARD_FLOW is set (iow, UPF_HARD_FLOW should imply
>     UPF_AUTO_CTS)
> 5. Check UPF_AUTO_CTS in uart_set_termios() (instead of UPF_HARD_FLOW),
>     but only when changing to CRTSCTS, and not from CRTSCTS.
> 6. Override the check in uart_port_startup() if UPF_AUTO_CTS.
>
> Other drivers that support auto CTS would also set UPF_AUTO_CTS; eg.,
> mxs_auart and bfin_uart (when CONFIG_SERIAL_BFIN_HARD_CTSRTS).
>
> UPF_HARD_FLOW should be scrapped, maybe along with the UART driver
> throttle/unthrottle methods (although I haven't really given any thought
> to handling in-band auto flow, aka UPF_SOFT_FLOW).
>
> Any driver doing auto RTS would then override RTS by whatever method
> required _whenever_ its set_mctrl() method indicates RTS has been lowered
> (and reenabled when RTS is raised).
>
> Right now, a driver doing UPF_HARD_FLOW via throttle/unthrottle doesn't
> behave properly when the TIOCMSET ioctl is used to manually flow control
> from userspace.
>
> A UPF_AUTO_RTS flag could be added to help the UART driver remember
> that its set_mctrl() method needs to virtualize RTS if required, like
> for uarts where MCR RTS = 0 does not override the auto RTS setting.
>
> Lastly, if a UART driver _knows_ that the remote is also wired for auto RTS
> (like via a firmware flag), then its set_mctrl() method could _also_ disable
> line status and data ready interrupts when RTS is lowered. UARTs with
> a big rx fifo could do this always.
>
> I would have already done this but I'm knee-deep in large patchsets for
> serial and tty already.
>
> Regards,
> Peter Hurley
Peter,

Thanks for the response. Let me review your response and get back to 
you. I also have access to one of the customer hardware where basic 
virtual serial port is used with h/w flow control and I would test your 
suggestion and let you know if I see any issues.

Murali
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
index 1928a3e..7705477 100644
--- a/Documentation/devicetree/bindings/serial/of-serial.txt
+++ b/Documentation/devicetree/bindings/serial/of-serial.txt
@@ -37,6 +37,7 @@  Optional properties:
 - auto-flow-control: one way to enable automatic flow control support. The
   driver is allowed to detect support for the capability even without this
   property.
+- has-hw-flow-control: the hardware has flow control capability.
 
 Example:
 
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 0e1bf88..b69aff2 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2338,9 +2338,11 @@  serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * the trigger, or the MCR RTS bit is cleared.  In the case where
 	 * the remote UART is not using CTS auto flow control, we must
 	 * have sufficient FIFO entries for the latency of the remote
-	 * UART to respond.  IOW, at least 32 bytes of FIFO.
+	 * UART to respond.  IOW, at least 32 bytes of FIFO. Also enable
+	 * AFE if hw flow control is supported
 	 */
-	if (up->capabilities & UART_CAP_AFE && port->fifosize >= 32) {
+	if ((up->capabilities & UART_CAP_AFE && (port->fifosize >= 32)) ||
+	    (port->flags & UPF_HARD_FLOW)) {
 		up->mcr &= ~UART_MCR_AFE;
 		if (termios->c_cflag & CRTSCTS)
 			up->mcr |= UART_MCR_AFE;
diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 9924660..77ec6a1 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -182,6 +182,10 @@  static int of_platform_serial_probe(struct platform_device *ofdev)
 					  "auto-flow-control"))
 			port8250.capabilities |= UART_CAP_AFE;
 
+		if (of_property_read_bool(ofdev->dev.of_node,
+					  "has-hw-flow-control"))
+			port8250.port.flags |= UPF_HARD_FLOW;
+
 		ret = serial8250_register_8250_port(&port8250);
 		break;
 	}
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b68550d..851707a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -174,8 +174,12 @@  static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 			if (tty->termios.c_cflag & CBAUD)
 				uart_set_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
 		}
-
-		if (tty_port_cts_enabled(port)) {
+		/*
+		 * if hw support flow control without software intervention,
+		 * then skip the below check
+		 */
+		if (tty_port_cts_enabled(port) &&
+		    !(uport->flags & UPF_HARD_FLOW)) {
 			spin_lock_irq(&uport->lock);
 			if (!(uport->ops->get_mctrl(uport) & TIOCM_CTS))
 				tty->hw_stopped = 1;
@@ -2772,7 +2776,9 @@  void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.cts++;
 
-	if (tty_port_cts_enabled(port)) {
+	/* skip below code if the hw flow control is supported */
+	if (tty_port_cts_enabled(port) &&
+	    !(uport->flags & UPF_HARD_FLOW)) {
 		if (tty->hw_stopped) {
 			if (status) {
 				tty->hw_stopped = 0;