| Message ID | 1458222449-12324-6-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Geert, On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote: > Replace the custom SCIx_HAVE_RTSCTS flag in the > plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in > the uart_port.flags and plat_sci_port.flags fields. > Remove the now unused plat_sci_port.capabilities field. > Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. UPF_HARD_FLOW is really for indicating the h/w supports automatic CTS and RTS signalling; ie., RTS is automatically de-asserted when rx fifo reaches some threshold and CTS will automatically prevent tx fifo output. There is no serial core flag for indicating the h/w supports (or does not) RTS and/or CTS signalling. Regards, Peter Hurley > Note that currently nothing sets the SCIx_HAVE_RTSCTS flag. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/tty/serial/sh-sci.c | 4 ++-- > include/linux/serial_sci.h | 6 ------ > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index 6897100ed5197df3..51b436e2334c3efc 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -720,7 +720,7 @@ static void sci_init_pins(struct uart_port *port, unsigned int cflag) > if (!reg->size) > return; > > - if ((s->cfg->capabilities & SCIx_HAVE_RTSCTS) && > + if ((port->flags & UPF_HARD_FLOW) && > ((!(cflag & CRTSCTS)))) { > unsigned short status; > > @@ -2247,7 +2247,7 @@ done: > if (reg->size) { > unsigned short ctrl = serial_port_in(port, SCFCR); > > - if (s->cfg->capabilities & SCIx_HAVE_RTSCTS) { > + if (port->flags & UPF_HARD_FLOW) { > if (termios->c_cflag & CRTSCTS) > ctrl |= SCFCR_MCE; > else > diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h > index 9f2bfd0557429ac3..95640ee68462190f 100644 > --- a/include/linux/serial_sci.h > +++ b/include/linux/serial_sci.h > @@ -48,17 +48,11 @@ struct plat_sci_port_ops { > }; > > /* > - * Port-specific capabilities > - */ > -#define SCIx_HAVE_RTSCTS BIT(0) > - > -/* > * Platform device specific platform_data struct > */ > struct plat_sci_port { > unsigned int type; /* SCI / SCIF / IRDA / HSCIF */ > upf_t flags; /* UPF_* flags */ > - unsigned long capabilities; /* Port features/capabilities */ > > unsigned int sampling_rate; > unsigned int scscr; /* SCSCR initialization */ > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote: > On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote: >> Replace the custom SCIx_HAVE_RTSCTS flag in the >> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in >> the uart_port.flags and plat_sci_port.flags fields. >> Remove the now unused plat_sci_port.capabilities field. >> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. > > UPF_HARD_FLOW is really for indicating the h/w supports automatic > CTS and RTS signalling; ie., RTS is automatically de-asserted when > rx fifo reaches some threshold and CTS will automatically prevent > tx fifo output. > > There is no serial core flag for indicating the h/w supports (or does not) > RTS and/or CTS signalling. Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins does have support for automatic RTS/CTS signalling. The driver has (unused) support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but it seems to be busted when enabled. If I understand it correctly: 1. Automatic RTS/CTS signalling should be enabled only if the user enabled it through termios->c_cflag & CRTSCTS, 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by the serial core, through .set_mctrl(), 3. Regardless of the user setting CRTSCTS or not, .get_mctrl() should report the current status of the CTS input pin. Are my assumptions correct? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote: > On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote: >>> Replace the custom SCIx_HAVE_RTSCTS flag in the >>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in >>> the uart_port.flags and plat_sci_port.flags fields. >>> Remove the now unused plat_sci_port.capabilities field. >>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. >> >> UPF_HARD_FLOW is really for indicating the h/w supports automatic >> CTS and RTS signalling; ie., RTS is automatically de-asserted when >> rx fifo reaches some threshold and CTS will automatically prevent >> tx fifo output. >> >> There is no serial core flag for indicating the h/w supports (or does not) >> RTS and/or CTS signalling. > > Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins > does have support for automatic RTS/CTS signalling. The driver has (unused) > support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but > it seems to be busted when enabled. Ok. So looking at the code, IIUC, SCIF does not provide a way to directly control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio). Is that correct? How to support autoRTS/autoCTS depends on the answer to this question. Is there a datasheet/trm that I can review describing register layout and uart behavior? > If I understand it correctly: > 1. Automatic RTS/CTS signalling should be enabled only if the user enabled it > through termios->c_cflag & CRTSCTS, yes > 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by > the serial core, through .set_mctrl(), Not quite. _Regardless of CRTSCTS setting_, the RTS output pin should be controlled through .set_mctrl() method. The serial core takes CRTSCTS into account on behalf of the driver when necessary. > 3. Regardless of the user setting CRTSCTS or not, .get_mctrl() should report > the current status of the CTS input pin. yes > Are my assumptions correct? A couple of things. gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not seeing any logic in these patches to prevent that. autoCTS without a way to read CTS input level means that although transmission stops, the driver has no way to update port->hw_stopped so the write buffer will keep filling up until it's full @ 4k. autoRTS without a way to override RTS output complicates throttling. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote: > On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote: >> On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >>> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote: >>>> Replace the custom SCIx_HAVE_RTSCTS flag in the >>>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in >>>> the uart_port.flags and plat_sci_port.flags fields. >>>> Remove the now unused plat_sci_port.capabilities field. >>>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. >>> >>> UPF_HARD_FLOW is really for indicating the h/w supports automatic >>> CTS and RTS signalling; ie., RTS is automatically de-asserted when >>> rx fifo reaches some threshold and CTS will automatically prevent >>> tx fifo output. >>> >>> There is no serial core flag for indicating the h/w supports (or does not) >>> RTS and/or CTS signalling. >> >> Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins >> does have support for automatic RTS/CTS signalling. The driver has (unused) >> support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but >> it seems to be busted when enabled. > > Ok. > > So looking at the code, IIUC, SCIF does not provide a way to directly > control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio). > Is that correct? > How to support autoRTS/autoCTS depends on the answer to this question. Actually it does have support for controlling RTS output and reading CTS input, but that is not yet implemented. Magnus posted a series touching this a while ago ("serial: sh-sci: Hardware flow control update", http://www.spinics.net/lists/linux-sh/msg38401.html), which received some comments. > Is there a datasheet/trm that I can review describing register layout and > uart behavior? I found a public one for an older SoC, see pages starting at (physical) page 849 http://documentation.renesas.com/doc/products/mpumcu/doc/superh/r01uh0048ej0200_sh7268.pdf - FIFO Control Register, bit MCE for automatic control - Serial Port Register for manual control >> 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by >> the serial core, through .set_mctrl(), > > Not quite. > > _Regardless of CRTSCTS setting_, the RTS output pin should be controlled > through .set_mctrl() method. The serial core takes CRTSCTS into account on > behalf of the driver when necessary. What does this mean exactly? AFAIU, there are three states: - Force RTS asserted, - Force RTS deasserted, - Use hardware-controlled automatic RTS, while .set_mctrl() just provides the boolean TIOCM_RTS flag? > gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not > seeing any logic in these patches to prevent that. Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet. I went for GPIO-based RTS/CTS first, as we now have a framework for that, and I can use it as a known-working base. > autoCTS without a way to read CTS input level means that although transmission > stops, the driver has no way to update port->hw_stopped so the write buffer > will keep filling up until it's full @ 4k. SCIF allows to read CTS input level, regardless of automatic RTS/CTS is enabled or not. > autoRTS without a way to override RTS output complicates throttling. SCIF allows to override RTS output. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote: > On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >> On 03/23/2016 02:33 AM, Geert Uytterhoeven wrote: >>> On Fri, Mar 18, 2016 at 8:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >>>> On 03/17/2016 06:47 AM, Geert Uytterhoeven wrote: >>>>> Replace the custom SCIx_HAVE_RTSCTS flag in the >>>>> plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in >>>>> the uart_port.flags and plat_sci_port.flags fields. >>>>> Remove the now unused plat_sci_port.capabilities field. >>>>> Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. >>>> >>>> UPF_HARD_FLOW is really for indicating the h/w supports automatic >>>> CTS and RTS signalling; ie., RTS is automatically de-asserted when >>>> rx fifo reaches some threshold and CTS will automatically prevent >>>> tx fifo output. >>>> >>>> There is no serial core flag for indicating the h/w supports (or does not) >>>> RTS and/or CTS signalling. >>> >>> Sorry for not being clearer: Renesas SCIF hardware with dedicated RTS/CTS pins >>> does have support for automatic RTS/CTS signalling. The driver has (unused) >>> support for that (cfr. setting the SCFCR_MCE (Modem Control Enable) flag), but >>> it seems to be busted when enabled. >> >> Ok. >> >> So looking at the code, IIUC, SCIF does not provide a way to directly >> control RTS output or read CTS input when RTS/CTS is pinned (ie, not gpio). >> Is that correct? >> How to support autoRTS/autoCTS depends on the answer to this question. > > Actually it does have support for controlling RTS output and reading CTS > input, but that is not yet implemented. > > Magnus posted a series touching this a while ago ("serial: sh-sci: Hardware > flow control update", http://www.spinics.net/lists/linux-sh/msg38401.html), > which received some comments. > >> Is there a datasheet/trm that I can review describing register layout and >> uart behavior? > > I found a public one for an older SoC, see pages starting at (physical) page 849 > http://documentation.renesas.com/doc/products/mpumcu/doc/superh/r01uh0048ej0200_sh7268.pdf Thanks. > > - FIFO Control Register, bit MCE for automatic control > - Serial Port Register for manual control Actually the FIFO Control Register doesn't say anything regarding automatic control, but 16.4.2 Operation in Asynchronous Mode, Section (3) Transmitting and Receiving Data gives examples of operation when "modem control is enabled". Those examples show autoRTS/autoCTS behavior. [discussion of set_mctrl() and autoRTS moved to EOM] >> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not >> seeing any logic in these patches to prevent that. > > Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet. Anything that enables the gpios which will be in DT and beyond our control needs to exclude autoRTS/autoCTS within the patch that provides the DT functionality. > I went for GPIO-based RTS/CTS first, as we now have a framework for that, > and I can use it as a known-working base. Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs. >> autoCTS without a way to read CTS input level means that although transmission >> stops, the driver has no way to update port->hw_stopped so the write buffer >> will keep filling up until it's full @ 4k. > > SCIF allows to read CTS input level, regardless of automatic RTS/CTS is > enabled or not. I see that but crucial support for CRTSCTS is missing, AFAICT; namely, a modem status interrupt. There's no way to determine when the remote re-enables CTS without polling, and the serial core provides no mechanism for the driver to poll CTS. So again, the driver has no way to trigger stopping/restarting tx when CTS changes without autoCTS. (A modem status interrupt for dCTS should call uart_handle_cts_change() to perform this operation). >> autoRTS without a way to override RTS output complicates throttling. > > SCIF allows to override RTS output. Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states "When the RTS pin is actually used as a port outputting the RTSDT bit value, the MCE bit in SCFCR should be cleared to 0." IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value outputting at the pin. >>> 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by >>> the serial core, through .set_mctrl(), >> >> Not quite. >> >> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled >> through .set_mctrl() method. The serial core takes CRTSCTS into account on >> behalf of the driver when necessary. > > What does this mean exactly? > AFAIU, there are three states: > - Force RTS asserted, > - Force RTS deasserted, > - Use hardware-controlled automatic RTS, > while .set_mctrl() just provides the boolean TIOCM_RTS flag? Since there is no userspace knob for autoRTS, drivers that enable autoRTS with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise) for set_mctrl(TIOCM_RTS). Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On Thu, Mar 24, 2016 at 1:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote: > On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote: >> On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >>> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not >>> seeing any logic in these patches to prevent that. >> >> Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet. > > Anything that enables the gpios which will be in DT and beyond our control > needs to exclude autoRTS/autoCTS within the patch that provides the > DT functionality. > >> I went for GPIO-based RTS/CTS first, as we now have a framework for that, >> and I can use it as a known-working base. > > Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to > contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs. AutoRTS/autoCTS cannot be enabled yet from DT. Hence on DT platforms, the driver uses either gpio-based RTS/CTS, or no RTS/CTS at all. >>> autoCTS without a way to read CTS input level means that although transmission >>> stops, the driver has no way to update port->hw_stopped so the write buffer >>> will keep filling up until it's full @ 4k. >> >> SCIF allows to read CTS input level, regardless of automatic RTS/CTS is >> enabled or not. > > I see that but crucial support for CRTSCTS is missing, AFAICT; namely, > a modem status interrupt. There's indeed no interrupt support for that, unless you use a GPIO. > There's no way to determine when the remote re-enables CTS without polling, > and the serial core provides no mechanism for the driver to poll CTS. OK, so it's the responsibility of the driver to poll CTS, if there's no CTS interrupt support. Even when autoCTS is enabled. > So again, the driver has no way to trigger stopping/restarting tx when CTS > changes without autoCTS. (A modem status interrupt for dCTS should call > uart_handle_cts_change() to perform this operation). But if the hardware has autoCTS, we should use it, right? Hence "... without autoCTS" is never true if the hardware has autoCTS? BTW, this means that drivers using mctrl_gpio_init_noauto(), but not implementing autoCTS(), and not polling CTS are broken? drivers/tty/serial/clps711x.c looks fishy: it doesn't poll CTS, but always sets TIOCM_CTS, probably to work around this? >>> autoRTS without a way to override RTS output complicates throttling. >> >> SCIF allows to override RTS output. > > Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states > "When the RTS pin is actually used as a port outputting > the RTSDT bit value, the MCE bit in SCFCR should be > cleared to 0." > > IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value > outputting at the pin. That's also my understanding: to manually control the RTS pin, you have to disable automatic flow control (or use pinmux to change the pin to a GPIO). [*] >>>> 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by >>>> the serial core, through .set_mctrl(), >>> >>> Not quite. >>> >>> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled >>> through .set_mctrl() method. The serial core takes CRTSCTS into account on >>> behalf of the driver when necessary. >> >> What does this mean exactly? >> AFAIU, there are three states: >> - Force RTS asserted, (let's call this state 1) >> - Force RTS deasserted, (state 2) >> - Use hardware-controlled automatic RTS, (state 3) >> while .set_mctrl() just provides the boolean TIOCM_RTS flag? > > Since there is no userspace knob for autoRTS, drivers that enable autoRTS > with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause > RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise) > for set_mctrl(TIOCM_RTS). OK, makes sense. That's also what I was guessing. So when autoRTS is enabled, we have either state 2 or state 3. Now, why would you want to override RTS output, and is [*] above an issue? Is it because you want autoCTS, but not autoRTS? Thanks for your answers, and sorry for still having that many questions... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On 03/24/2016 06:21 AM, Geert Uytterhoeven wrote: > On Thu, Mar 24, 2016 at 1:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote: >> On 03/23/2016 01:00 PM, Geert Uytterhoeven wrote: >>> On Wed, Mar 23, 2016 at 6:11 PM, Peter Hurley <peter@hurleysoftware.com> wrote: >>>> gpio-based RTS/CTS is mutually exclusive with autoRTS/autoCTS. But I'm not >>>> seeing any logic in these patches to prevent that. >>> >>> Sure. Currently nothing sets SCIx_HAVE_RTSCTS, so this can't happen yet. >> >> Anything that enables the gpios which will be in DT and beyond our control >> needs to exclude autoRTS/autoCTS within the patch that provides the >> DT functionality. >> >>> I went for GPIO-based RTS/CTS first, as we now have a framework for that, >>> and I can use it as a known-working base. >> >> Right. And I'm saying that a condition of gpio-based RTS/CTS support needs to >> contain the logic that prevents autoRTS/autoCTS, and not rely on orthogonal knobs. > > AutoRTS/autoCTS cannot be enabled yet from DT. > Hence on DT platforms, the driver uses either gpio-based RTS/CTS, or no RTS/CTS > at all. I do understand that the situation is mutually exclusive currently, and even after the gpio patches. I'm probably just being overly-cautious here. There have been a couple of situations where a DT configuration was invalid and some ugly hoops were required to prevent it in the driver; eg., famously the OMAP dma fubar. I guess I'd be okay with skipping it as long as I knew someone was going to make sure it got addressed soonish. >>>> autoCTS without a way to read CTS input level means that although transmission >>>> stops, the driver has no way to update port->hw_stopped so the write buffer >>>> will keep filling up until it's full @ 4k. >>> >>> SCIF allows to read CTS input level, regardless of automatic RTS/CTS is >>> enabled or not. >> >> I see that but crucial support for CRTSCTS is missing, AFAICT; namely, >> a modem status interrupt. > > There's indeed no interrupt support for that, unless you use a GPIO. > >> There's no way to determine when the remote re-enables CTS without polling, >> and the serial core provides no mechanism for the driver to poll CTS. > > OK, so it's the responsibility of the driver to poll CTS, if there's no CTS > interrupt support. Sorry, no, I didn't mean the driver should try to implement some polling scheme on its own. > Even when autoCTS is enabled. For autoCTS when the driver/hardware does not have CTS interrupt support, set UPSTAT_AUTOCTS in port->status. This will prevent the serial core from stopping the tty if CTS is not active (since the driver/hardware cannot restart the tty when CTS is set active). For !autoCTS when the driver/hardware does not have CTS interrupt support, get_mctrl() must always report CTS active. [Another potential solution is to disable CRTSCTS in the driver's set_termios method: this would be preferable but I'm not sure all userspace will be ok with this.] >> So again, the driver has no way to trigger stopping/restarting tx when CTS >> changes without autoCTS. (A modem status interrupt for dCTS should call >> uart_handle_cts_change() to perform this operation). > > But if the hardware has autoCTS, we should use it, right? Yes, but I consider that a refinement over basic CTS/RTS handling, and so not an actual driver requirement. > Hence "... without autoCTS" is never true if the hardware has autoCTS? Well, a driver can only support s/w assisted h/w flow control*, even though the h/w is capable of autoCTS. * enabling/disabling tx on dCTS interrupt > BTW, this means that drivers using mctrl_gpio_init_noauto(), but not > implementing autoCTS(), and not polling CTS are broken? > > drivers/tty/serial/clps711x.c looks fishy: This looks broken. If CRTSCTS is set and CTS is pinned to a gpio, tx will continue even when the CTS gpio is inactive. Also, if the CTS gpio is inactive when the tty is opened or the termios is changed, tx will be stuck off. > it doesn't poll CTS, but always sets TIOCM_CTS, probably to work around this? This driver is not always setting TIOCM_CTS; that's just the default if CTS is not pinned to a gpio. >>>> autoRTS without a way to override RTS output complicates throttling. >>> >>> SCIF allows to override RTS output. >> >> Not the way I read the TRM: 16.3.11 Serial Port Register, bit 7 states >> "When the RTS pin is actually used as a port outputting >> the RTSDT bit value, the MCE bit in SCFCR should be >> cleared to 0." >> >> IOW, autoRTS/autoCTS needs to be disabled to get the RTS override value >> outputting at the pin. > > That's also my understanding: to manually control the RTS pin, you have to > disable automatic flow control (or use pinmux to change the pin to a GPIO). > > [*] > >>>>> 2. If the user didn't set CRTSCTS, the RTS output pin should be controlled by >>>>> the serial core, through .set_mctrl(), >>>> >>>> Not quite. >>>> >>>> _Regardless of CRTSCTS setting_, the RTS output pin should be controlled >>>> through .set_mctrl() method. The serial core takes CRTSCTS into account on >>>> behalf of the driver when necessary. >>> >>> What does this mean exactly? >>> AFAIU, there are three states: >>> - Force RTS asserted, > > (let's call this state 1) > >>> - Force RTS deasserted, > > (state 2) > >>> - Use hardware-controlled automatic RTS, > > (state 3) > >>> while .set_mctrl() just provides the boolean TIOCM_RTS flag? >> >> Since there is no userspace knob for autoRTS, drivers that enable autoRTS >> with CRTSCTS should force RTS to inactive for set_mctrl(!TIOCM_RTS) and cause >> RTS to return to normal operation (autoRTS, if enabled, or RTS active otherwise) >> for set_mctrl(TIOCM_RTS). > > OK, makes sense. That's also what I was guessing. So when autoRTS is enabled, > we have either state 2 or state 3. Yes. > Now, why would you want to override RTS output, A couple of reasons: 1. userspace terminal control will expect to be able to use TIOCMSET/TIOCMBIC/TIOCMBIS to stop/resume input by dropping RTS when using CRTSCTS. 2. userspace drivers for uart-interfaced devices use TIOCMSET on RTS for things like device wakeup. 3. in-tree drivers use tiocmset() directly for the same thing. > and is [*] above an issue? Yes. The driver must disable autoRTS in set_mctrl() if TIOCM_RTS is clear and restore the autoRTS/RTS state if TIOCM_RTS is set. The omap8250 driver does this (omap8250_set_mctrl()). IMPORTANT NOTE: do _not_ use UPSTAT_AUTORTS to track your autoRTS state, like the omap8250 driver does. If you do, you need to provide throttle/unthrottle methods to stop and resume remote input; however, I don't think the methods currently used by these drivers is appropriate or safe (by letting the rx fifo fill up). > Is it because you want autoCTS, but not autoRTS? No, but that does happen. > Thanks for your answers, and sorry for still having that many questions... Not your fault. I should really document this. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 6897100ed5197df3..51b436e2334c3efc 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -720,7 +720,7 @@ static void sci_init_pins(struct uart_port *port, unsigned int cflag) if (!reg->size) return; - if ((s->cfg->capabilities & SCIx_HAVE_RTSCTS) && + if ((port->flags & UPF_HARD_FLOW) && ((!(cflag & CRTSCTS)))) { unsigned short status; @@ -2247,7 +2247,7 @@ done: if (reg->size) { unsigned short ctrl = serial_port_in(port, SCFCR); - if (s->cfg->capabilities & SCIx_HAVE_RTSCTS) { + if (port->flags & UPF_HARD_FLOW) { if (termios->c_cflag & CRTSCTS) ctrl |= SCFCR_MCE; else diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h index 9f2bfd0557429ac3..95640ee68462190f 100644 --- a/include/linux/serial_sci.h +++ b/include/linux/serial_sci.h @@ -48,17 +48,11 @@ struct plat_sci_port_ops { }; /* - * Port-specific capabilities - */ -#define SCIx_HAVE_RTSCTS BIT(0) - -/* * Platform device specific platform_data struct */ struct plat_sci_port { unsigned int type; /* SCI / SCIF / IRDA / HSCIF */ upf_t flags; /* UPF_* flags */ - unsigned long capabilities; /* Port features/capabilities */ unsigned int sampling_rate; unsigned int scscr; /* SCSCR initialization */
Replace the custom SCIx_HAVE_RTSCTS flag in the plat_sci_port.capabilities field by the standard UPF_HARD_FLOW flag in the uart_port.flags and plat_sci_port.flags fields. Remove the now unused plat_sci_port.capabilities field. Legacy pllatform data can enable UPF_HARD_FLOW in plat_sci_port.flags. Note that currently nothing sets the SCIx_HAVE_RTSCTS flag. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/tty/serial/sh-sci.c | 4 ++-- include/linux/serial_sci.h | 6 ------ 2 files changed, 2 insertions(+), 8 deletions(-)