| Message ID | 1398971093-17164-1-git-send-email-m-karicheri2@ti.com |
|---|---|
| State | Accepted |
| Commit | 06aa82e498c144c7784a6f3d3b55458b272d6146 |
| Headers | show |
>-----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/
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/
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/
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
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
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
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/
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
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/
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/
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
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
[ +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
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
> 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/
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
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/
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/
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
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
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
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
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 --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;
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(-)