* [PATCH] serial: samsung: add support for manual RTS setting @ 2013-09-11 10:08 José Miguel Gonçalves 2013-09-17 10:18 ` Tomasz Figa 0 siblings, 1 reply; 6+ messages in thread From: José Miguel Gonçalves @ 2013-09-11 10:08 UTC (permalink / raw) To: linux-samsung-soc Cc: linux-kernel, linux-serial, José Miguel Gonçalves The Samsung serial driver currently does not support setting the RTS pin with an ioctl(TIOCMSET) call. This patch adds this support. Signed-off-by: José Miguel Gonçalves <jose.goncalves@inov.pt> --- drivers/tty/serial/samsung.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index f3dfa19..e5dd808 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c @@ -407,7 +407,14 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port) static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl) { - /* todo - possibly remove AFC and do manual CTS */ + unsigned int umcon = rd_regl(port, S3C2410_UMCON); + + if (mctrl & TIOCM_RTS) + umcon |= S3C2410_UMCOM_RTS_LOW; + else + umcon &= ~S3C2410_UMCOM_RTS_LOW; + + wr_regl(port, S3C2410_UMCON, umcon); } static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state) @@ -774,8 +781,6 @@ static void s3c24xx_serial_set_termios(struct uart_port *port, if (termios->c_cflag & CSTOPB) ulcon |= S3C2410_LCON_STOPB; - umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0; - if (termios->c_cflag & PARENB) { if (termios->c_cflag & PARODD) ulcon |= S3C2410_LCON_PODD; @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct uart_port *port, wr_regl(port, S3C2410_ULCON, ulcon); wr_regl(port, S3C2410_UBRDIV, quot); + + if (termios->c_cflag & CRTSCTS) + umcon = S3C2410_UMCOM_AFC; + else + umcon = rd_regb(port, S3C2410_UMCON) & ~S3C2410_UMCOM_AFC; + wr_regl(port, S3C2410_UMCON, umcon); if (ourport->info->has_divslot) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: samsung: add support for manual RTS setting 2013-09-11 10:08 [PATCH] serial: samsung: add support for manual RTS setting José Miguel Gonçalves @ 2013-09-17 10:18 ` Tomasz Figa 2013-09-17 13:03 ` José Miguel Gonçalves 0 siblings, 1 reply; 6+ messages in thread From: Tomasz Figa @ 2013-09-17 10:18 UTC (permalink / raw) To: José Miguel Gonçalves Cc: linux-samsung-soc, linux-kernel, linux-serial Hi José, Please see my comments below. On Wednesday 11 of September 2013 11:08:27 José Miguel Gonçalves wrote: > The Samsung serial driver currently does not support setting the > RTS pin with an ioctl(TIOCMSET) call. This patch adds this support. > > Signed-off-by: José Miguel Gonçalves <jose.goncalves@inov.pt> > --- > drivers/tty/serial/samsung.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c > index f3dfa19..e5dd808 100644 > --- a/drivers/tty/serial/samsung.c > +++ b/drivers/tty/serial/samsung.c > @@ -407,7 +407,14 @@ static unsigned int s3c24xx_serial_get_mctrl(struct > uart_port *port) > > static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned > int mctrl) { > - /* todo - possibly remove AFC and do manual CTS */ > + unsigned int umcon = rd_regl(port, S3C2410_UMCON); > + > + if (mctrl & TIOCM_RTS) > + umcon |= S3C2410_UMCOM_RTS_LOW; > + else > + umcon &= ~S3C2410_UMCOM_RTS_LOW; > + > + wr_regl(port, S3C2410_UMCON, umcon); I wonder if port capability shouldn't be considered here. Depending on SoC, only selected ports provide modem control capability. For example on S3C64xx only ports 0 and 1 support modem control, while ports 2 and 3 don't. > } > > static void s3c24xx_serial_break_ctl(struct uart_port *port, int > break_state) @@ -774,8 +781,6 @@ static void > s3c24xx_serial_set_termios(struct uart_port *port, if (termios->c_cflag > & CSTOPB) > ulcon |= S3C2410_LCON_STOPB; > > - umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0; > - > if (termios->c_cflag & PARENB) { > if (termios->c_cflag & PARODD) > ulcon |= S3C2410_LCON_PODD; > @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct > uart_port *port, > > wr_regl(port, S3C2410_ULCON, ulcon); > wr_regl(port, S3C2410_UBRDIV, quot); > + > + if (termios->c_cflag & CRTSCTS) > + umcon = S3C2410_UMCOM_AFC; Is it correct to override the last manual RTS value set to this register when activating manual flow control? Shouldn't the code be more like the following: umcon = rd_regb(port, S3C2410_UMCON); if (termios->c_cflag & CRTSCTS) umcon |= S3C2410_UMCOM_AFC; else umcon &= ~S3C2410_UMCOM_AFC; wr_regl(port, S3C2410_UMCON, umcon); Probably port capability should be considered here as well. Best regards, Tomasz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: samsung: add support for manual RTS setting 2013-09-17 10:18 ` Tomasz Figa @ 2013-09-17 13:03 ` José Miguel Gonçalves 2013-09-17 15:21 ` Tomasz Figa 0 siblings, 1 reply; 6+ messages in thread From: José Miguel Gonçalves @ 2013-09-17 13:03 UTC (permalink / raw) To: Tomasz Figa Cc: linux-samsung-soc, linux-kernel, linux-serial, Heiko Stübner Hi Tomasz, On 17-09-2013 11:18, Tomasz Figa wrote: > Hi José, > > Please see my comments below. > > On Wednesday 11 of September 2013 11:08:27 José Miguel Gonçalves wrote: >> The Samsung serial driver currently does not support setting the >> RTS pin with an ioctl(TIOCMSET) call. This patch adds this support. >> >> Signed-off-by: José Miguel Gonçalves <jose.goncalves@inov.pt> >> --- >> drivers/tty/serial/samsung.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c >> index f3dfa19..e5dd808 100644 >> --- a/drivers/tty/serial/samsung.c >> +++ b/drivers/tty/serial/samsung.c >> @@ -407,7 +407,14 @@ static unsigned int s3c24xx_serial_get_mctrl(struct >> uart_port *port) >> >> static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned >> int mctrl) { >> - /* todo - possibly remove AFC and do manual CTS */ >> + unsigned int umcon = rd_regl(port, S3C2410_UMCON); >> + >> + if (mctrl & TIOCM_RTS) >> + umcon |= S3C2410_UMCOM_RTS_LOW; >> + else >> + umcon &= ~S3C2410_UMCOM_RTS_LOW; >> + >> + wr_regl(port, S3C2410_UMCON, umcon); > I wonder if port capability shouldn't be considered here. Depending on SoC, > only selected ports provide modem control capability. > > For example on S3C64xx only ports 0 and 1 support modem control, while > ports 2 and 3 don't. Same for S3C2416. I also wondered that, but while I have information for all S3C24xx chips and for those a simple test ( port->line < 2) would validate this, I don't know about other SoCs this driver supports. Bearing this in mind and also that the current implementation of s3c24xx_serial_get_mctrl() does not check also for which port it applies, I opted for this solution. > >> } >> >> static void s3c24xx_serial_break_ctl(struct uart_port *port, int >> break_state) @@ -774,8 +781,6 @@ static void >> s3c24xx_serial_set_termios(struct uart_port *port, if (termios->c_cflag >> & CSTOPB) >> ulcon |= S3C2410_LCON_STOPB; >> >> - umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0; >> - >> if (termios->c_cflag & PARENB) { >> if (termios->c_cflag & PARODD) >> ulcon |= S3C2410_LCON_PODD; >> @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct >> uart_port *port, >> >> wr_regl(port, S3C2410_ULCON, ulcon); >> wr_regl(port, S3C2410_UBRDIV, quot); >> + >> + if (termios->c_cflag & CRTSCTS) >> + umcon = S3C2410_UMCOM_AFC; > Is it correct to override the last manual RTS value set to this register > when activating manual flow control? > > Shouldn't the code be more like the following: > > umcon = rd_regb(port, S3C2410_UMCON); > if (termios->c_cflag & CRTSCTS) > umcon |= S3C2410_UMCOM_AFC; > else > umcon &= ~S3C2410_UMCOM_AFC; > wr_regl(port, S3C2410_UMCON, umcon); > > Probably port capability should be considered here as well. > Looking at the S3C24xx user manuals I've seen that if you set the automatic flow control (AFC) with the S3C2410_UMCOM_AFC mask, the UART controller ignores the manual RTS setting value with the S3C2410_UMCOM_RTS_LOW bitmask, so it is not necessary to do that. Also, the upper bits of UMCON control the FIFO level to trigger the AFC and you should initialize these bits when using AFC (I've set these to 0 to use full FIFO, as it was previously). Regarding port capability, if it's decided to validate it in s3c24xx_serial_get_mctrl() and s3c24xx_serial_set_mctrl() it should also be validated here. The question is how to validate for the full spectrum of SoCs that this driver supports? Best regards, José Gonçalves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: samsung: add support for manual RTS setting 2013-09-17 13:03 ` José Miguel Gonçalves @ 2013-09-17 15:21 ` Tomasz Figa 2013-09-17 19:08 ` José Miguel Gonçalves 0 siblings, 1 reply; 6+ messages in thread From: Tomasz Figa @ 2013-09-17 15:21 UTC (permalink / raw) To: José Miguel Gonçalves, Greg Kroah-Hartman Cc: linux-samsung-soc, linux-kernel, linux-serial, Heiko Stübner, Sylwester Nawrocki [Ccing Greg and Sylwester] On Tuesday 17 of September 2013 14:03:35 José Miguel Gonçalves wrote: > Hi Tomasz, > > On 17-09-2013 11:18, Tomasz Figa wrote: > > Hi José, > > > > Please see my comments below. > > > > On Wednesday 11 of September 2013 11:08:27 José Miguel Gonçalves wrote: > >> The Samsung serial driver currently does not support setting the > >> RTS pin with an ioctl(TIOCMSET) call. This patch adds this support. > >> > >> Signed-off-by: José Miguel Gonçalves <jose.goncalves@inov.pt> > >> --- > >> > >> drivers/tty/serial/samsung.c | 17 ++++++++++++++--- > >> 1 file changed, 14 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/tty/serial/samsung.c > >> b/drivers/tty/serial/samsung.c > >> index f3dfa19..e5dd808 100644 > >> --- a/drivers/tty/serial/samsung.c > >> +++ b/drivers/tty/serial/samsung.c > >> @@ -407,7 +407,14 @@ static unsigned int > >> s3c24xx_serial_get_mctrl(struct > >> uart_port *port) > >> > >> static void s3c24xx_serial_set_mctrl(struct uart_port *port, > >> unsigned > >> > >> int mctrl) { > >> - /* todo - possibly remove AFC and do manual CTS */ > >> + unsigned int umcon = rd_regl(port, S3C2410_UMCON); > >> + > >> + if (mctrl & TIOCM_RTS) > >> + umcon |= S3C2410_UMCOM_RTS_LOW; > >> + else > >> + umcon &= ~S3C2410_UMCOM_RTS_LOW; > >> + > >> + wr_regl(port, S3C2410_UMCON, umcon); > > > > I wonder if port capability shouldn't be considered here. Depending on > > SoC, only selected ports provide modem control capability. > > > > For example on S3C64xx only ports 0 and 1 support modem control, while > > ports 2 and 3 don't. > > Same for S3C2416. I also wondered that, but while I have information for > all S3C24xx chips and for those a simple test ( port->line < 2) would > validate this, I don't know about other SoCs this driver supports. > Bearing this in mind and also that the current implementation of > s3c24xx_serial_get_mctrl() does not check also for which port it > applies, I opted for this solution. See below. > >> } > >> > >> static void s3c24xx_serial_break_ctl(struct uart_port *port, int > >> > >> break_state) @@ -774,8 +781,6 @@ static void > >> s3c24xx_serial_set_termios(struct uart_port *port, if > >> (termios->c_cflag > >> & CSTOPB) > >> > >> ulcon |= S3C2410_LCON_STOPB; > >> > >> - umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0; > >> - > >> > >> if (termios->c_cflag & PARENB) { > >> > >> if (termios->c_cflag & PARODD) > >> > >> ulcon |= S3C2410_LCON_PODD; > >> > >> @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct > >> uart_port *port, > >> > >> wr_regl(port, S3C2410_ULCON, ulcon); > >> wr_regl(port, S3C2410_UBRDIV, quot); > >> > >> + > >> + if (termios->c_cflag & CRTSCTS) > >> + umcon = S3C2410_UMCOM_AFC; > > > > Is it correct to override the last manual RTS value set to this > > register > > when activating manual flow control? > > > > Shouldn't the code be more like the following: > > umcon = rd_regb(port, S3C2410_UMCON); > > if (termios->c_cflag & CRTSCTS) > > > > umcon |= S3C2410_UMCOM_AFC; > > > > else > > > > umcon &= ~S3C2410_UMCOM_AFC; > > > > wr_regl(port, S3C2410_UMCON, umcon); > > > > Probably port capability should be considered here as well. > > Looking at the S3C24xx user manuals I've seen that if you set the > automatic flow control (AFC) with the S3C2410_UMCOM_AFC mask, the UART > controller ignores the manual RTS setting value with the > S3C2410_UMCOM_RTS_LOW bitmask, so it is not necessary to do that. Also, > the upper bits of UMCON control the FIFO level to trigger the AFC and > you should initialize these bits when using AFC (I've set these to 0 to > use full FIFO, as it was previously). I had the following scenario in mind: 1) enable CRTSCTS, 2) set RTS status using the IOCTL, 3) disable CRTSCTS, 4) do something, 5) enable CRTSCTS again. I would expect that the value set in point 2 would be still valid after point 5, while it will be reset. > Regarding port capability, if it's decided to validate it in > s3c24xx_serial_get_mctrl() and s3c24xx_serial_set_mctrl() it should also > be validated here. The question is how to validate for the full spectrum > of SoCs that this driver supports? Hmm, since the driver is already broken in this aspect, ignoring this in your patch might be fine for now. A follow up patch fixing this would be welcome, though. However I don't have any good idea how to implement this at the moment. First thing that comes to my mind is using the variant data structures to store information about per port capability (different port FIFO sizes are already handled like this), but this would imply splitting some of the groups, as S5PC100 supports modem control for different subset of ports than S3C64xx, while they both use the same variant data. Using device tree, this could be passed as an extra property, but some of the platforms using samsung serial driver can be booted without device tree, so it wouldn't cover all the cases. Best regards, Tomasz P.S. Please remember to add all the relevant people to Cc when sending patches. You can use scripts/get_maintainer.pl to find them. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: samsung: add support for manual RTS setting 2013-09-17 15:21 ` Tomasz Figa @ 2013-09-17 19:08 ` José Miguel Gonçalves 2013-09-18 9:40 ` Tomasz Figa 0 siblings, 1 reply; 6+ messages in thread From: José Miguel Gonçalves @ 2013-09-17 19:08 UTC (permalink / raw) To: Tomasz Figa Cc: Greg Kroah-Hartman, linux-samsung-soc, linux-kernel, linux-serial, Heiko Stübner, Sylwester Nawrocki On 17-09-2013 16:21, Tomasz Figa wrote: > I had the following scenario in mind: > 1) enable CRTSCTS, > 2) set RTS status using the IOCTL, > 3) disable CRTSCTS, > 4) do something, > 5) enable CRTSCTS again. > > I would expect that the value set in point 2 would be still valid after > point 5, while it will be reset. Maybe I'm missing something but, as I see it, as soon as you enable AFC in the UART controller the RTS pin is no more manually controllable, so after point 5 the pin is set automattically by the controller according with the Rx FIFO contents. Don't you want to say instead that the value set in 2) will be valid in 4)? > >> Regarding port capability, if it's decided to validate it in >> s3c24xx_serial_get_mctrl() and s3c24xx_serial_set_mctrl() it should also >> be validated here. The question is how to validate for the full spectrum >> of SoCs that this driver supports? > Hmm, since the driver is already broken in this aspect, ignoring this in > your patch might be fine for now. A follow up patch fixing this would be > welcome, though. However I don't have any good idea how to implement this > at the moment. > > First thing that comes to my mind is using the variant data structures to > store information about per port capability (different port FIFO sizes are > already handled like this), but this would imply splitting some of the > groups, as S5PC100 supports modem control for different subset of ports > than S3C64xx, while they both use the same variant data. > > Using device tree, this could be passed as an extra property, but some of > the platforms using samsung serial driver can be booted without device > tree, so it wouldn't cover all the cases. So, as I understand, for now is enough to resubmit the patch with the UMCON register setting in s3c24xx_serial_set_termios() preserving the previous manual setting of the RTS pin, correct? I was thinking in something like this: umcon = rd_regb(port, S3C2410_UMCON); if (termios->c_cflag & CRTSCTS) { umcon |= S3C2410_UMCOM_AFC; umcon &= ~S3C2412_UMCON_AFC_8; // Disable RTS when RX FIFO contains 63 bytes } else { umcon &= ~S3C2410_UMCOM_AFC; } wr_regl(port, S3C2410_UMCON, umcon); > Best regards, > Tomasz > > P.S. Please remember to add all the relevant people to Cc when sending > patches. You can use scripts/get_maintainer.pl to find them. OK, sorry for that. Bet regards, José Gonçalves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: samsung: add support for manual RTS setting 2013-09-17 19:08 ` José Miguel Gonçalves @ 2013-09-18 9:40 ` Tomasz Figa 0 siblings, 0 replies; 6+ messages in thread From: Tomasz Figa @ 2013-09-18 9:40 UTC (permalink / raw) To: José Miguel Gonçalves Cc: Greg Kroah-Hartman, linux-samsung-soc, linux-kernel, linux-serial, Heiko Stübner, Sylwester Nawrocki On Tuesday 17 of September 2013 20:08:33 José Miguel Gonçalves wrote: > On 17-09-2013 16:21, Tomasz Figa wrote: > > I had the following scenario in mind: > > 1) enable CRTSCTS, > > 2) set RTS status using the IOCTL, > > 3) disable CRTSCTS, > > 4) do something, > > 5) enable CRTSCTS again. > > > > I would expect that the value set in point 2 would be still valid after > > point 5, while it will be reset. > > Maybe I'm missing something but, as I see it, as soon as you enable AFC > in the UART controller the RTS pin is no more manually controllable, so > after point 5 the pin is set automattically by the controller according > with the Rx FIFO contents. Don't you want to say instead that the value > set in 2) will be valid in 4)? Ahh, right, I inverted CRTSCTS bit polarity in points 1, 3 and 5. It should be: 1) disable CRTSCTS, 2) set RTS status using the IOCTL, 3) enable CRTSCTS, 4) do something, 5) disable CRTSCTS again. > >> Regarding port capability, if it's decided to validate it in > >> s3c24xx_serial_get_mctrl() and s3c24xx_serial_set_mctrl() it should > >> also > >> be validated here. The question is how to validate for the full > >> spectrum > >> of SoCs that this driver supports? > > > > Hmm, since the driver is already broken in this aspect, ignoring this > > in > > your patch might be fine for now. A follow up patch fixing this would > > be > > welcome, though. However I don't have any good idea how to implement > > this at the moment. > > > > First thing that comes to my mind is using the variant data structures > > to store information about per port capability (different port FIFO > > sizes are already handled like this), but this would imply splitting > > some of the groups, as S5PC100 supports modem control for different > > subset of ports than S3C64xx, while they both use the same variant > > data. > > > > Using device tree, this could be passed as an extra property, but some > > of the platforms using samsung serial driver can be booted without > > device tree, so it wouldn't cover all the cases. > > So, as I understand, for now is enough to resubmit the patch with the > UMCON register setting in s3c24xx_serial_set_termios() preserving the > previous manual setting of the RTS pin, correct? I was thinking in > something like this: > > umcon = rd_regb(port, S3C2410_UMCON); > if (termios->c_cflag & CRTSCTS) { > umcon |= S3C2410_UMCOM_AFC; > umcon &= ~S3C2412_UMCON_AFC_8; // Disable RTS when RX FIFO > contains 63 bytes } else { > umcon &= ~S3C2410_UMCOM_AFC; > } > wr_regl(port, S3C2410_UMCON, umcon); Yes, this looks fine. Best regards, Tomasz ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-18 9:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-11 10:08 [PATCH] serial: samsung: add support for manual RTS setting José Miguel Gonçalves 2013-09-17 10:18 ` Tomasz Figa 2013-09-17 13:03 ` José Miguel Gonçalves 2013-09-17 15:21 ` Tomasz Figa 2013-09-17 19:08 ` José Miguel Gonçalves 2013-09-18 9:40 ` Tomasz Figa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox