Re: [PATCH] serial: samsung: add support for manual RTS setting

[Tomasz Figa <t.figa@samsung.com>]

[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.