[PATCH V2 0/3] Add RS-485 support and fix RTS/CTS implementation

[PATCH V2 0/3] Add RS-485 support and fix RTS/CTS implementation

[Bhuvanchandra DV]

Changes since V1:
Use common get/set mctrl control methods for both lpuart, lpuart32
and fix indention.

Bhuvanchandra DV (3):
  ARM: dts: colibri-vf: Add pinmux for UART_0 aka UART_A RTS/CTS pins
  tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl
  tty: serial: fsl_lpuart: Add support for RS-485

 arch/arm/boot/dts/vf-colibri.dtsi |   2 +
 drivers/tty/serial/fsl_lpuart.c   | 103 ++++++++++++++++++++------------------
 2 files changed, 57 insertions(+), 48 deletions(-)

-- 
2.1.0

[PATCH V2 1/3] ARM: dts: colibri-vf: Add pinmux for UART_0 aka UART_A RTS/CTS pins

[Bhuvanchandra DV]

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 arch/arm/boot/dts/vf-colibri.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/vf-colibri.dtsi b/arch/arm/boot/dts/vf-colibri.dtsi
index 68ca125..ad6c5ca 100644
--- a/arch/arm/boot/dts/vf-colibri.dtsi
+++ b/arch/arm/boot/dts/vf-colibri.dtsi
@@ -174,6 +174,8 @@
 			fsl,pins = <
 				VF610_PAD_PTB10__UART0_TX		0x21a2
 				VF610_PAD_PTB11__UART0_RX		0x21a1
+				VF610_PAD_PTB12__UART0_RTS		0x21a2
+				VF610_PAD_PTB13__UART0_CTS		0x21a1
 			>;
 		};
 
-- 
2.1.0

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Bhuvanchandra DV]

The LPUART does not provide manual control of RTS/CTS signals,
those can only be controlled by the hardware directly. Therefore
manual control of those signals through mctrl can not be provided.
The current implementation enables/disables the automatic control,
which is not what mctrl should do, hence remove the incorrect
implementation.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------
 1 file changed, 7 insertions(+), 56 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 08ce76f..532cfb7 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
 
 static unsigned int lpuart_get_mctrl(struct uart_port *port)
 {
-	unsigned int temp = 0;
-	unsigned char reg;
-
-	reg = readb(port->membase + UARTMODEM);
-	if (reg & UARTMODEM_TXCTSE)
-		temp |= TIOCM_CTS;
-
-	if (reg & UARTMODEM_RXRTSE)
-		temp |= TIOCM_RTS;
-
-	return temp;
-}
-
-static unsigned int lpuart32_get_mctrl(struct uart_port *port)
-{
-	unsigned int temp = 0;
-	unsigned long reg;
-
-	reg = lpuart32_read(port->membase + UARTMODIR);
-	if (reg & UARTMODIR_TXCTSE)
-		temp |= TIOCM_CTS;
-
-	if (reg & UARTMODIR_RXRTSE)
-		temp |= TIOCM_RTS;
-
-	return temp;
+	return TIOCM_CAR | TIOCM_CTS | TIOCM_DSR;
 }
 
 static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	unsigned char temp;
-
-	temp = readb(port->membase + UARTMODEM) &
-			~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
-
-	if (mctrl & TIOCM_RTS)
-		temp |= UARTMODEM_RXRTSE;
-
-	if (mctrl & TIOCM_CTS)
-		temp |= UARTMODEM_TXCTSE;
-
-	writeb(temp, port->membase + UARTMODEM);
-}
-
-static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl)
-{
-	unsigned long temp;
-
-	temp = lpuart32_read(port->membase + UARTMODIR) &
-			~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
-
-	if (mctrl & TIOCM_RTS)
-		temp |= UARTMODIR_RXRTSE;
-
-	if (mctrl & TIOCM_CTS)
-		temp |= UARTMODIR_TXCTSE;
-
-	lpuart32_write(temp, port->membase + UARTMODIR);
+    /*
+     * CTS/RTS can _only_ be handled
+     * automatically by the hardware.
+     */
 }
 
 static void lpuart_break_ctl(struct uart_port *port, int break_state)
@@ -1515,8 +1466,8 @@ static struct uart_ops lpuart_pops = {
 
 static struct uart_ops lpuart32_pops = {
 	.tx_empty	= lpuart32_tx_empty,
-	.set_mctrl	= lpuart32_set_mctrl,
-	.get_mctrl	= lpuart32_get_mctrl,
+	.set_mctrl	= lpuart_set_mctrl,
+	.get_mctrl	= lpuart_get_mctrl,
 	.stop_tx	= lpuart32_stop_tx,
 	.start_tx	= lpuart32_start_tx,
 	.stop_rx	= lpuart32_stop_rx,
-- 
2.1.0

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Uwe Kleine-König]

Hello,

On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote:
> The LPUART does not provide manual control of RTS/CTS signals,
> those can only be controlled by the hardware directly. Therefore
> manual control of those signals through mctrl can not be provided.
> The current implementation enables/disables the automatic control,
> which is not what mctrl should do, hence remove the incorrect
> implementation.
> 
> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------
>  1 file changed, 7 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 08ce76f..532cfb7 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
>  
>  static unsigned int lpuart_get_mctrl(struct uart_port *port)
>  {
> -	unsigned int temp = 0;
> -	unsigned char reg;
> -
> -	reg = readb(port->membase + UARTMODEM);
> -	if (reg & UARTMODEM_TXCTSE)
> -		temp |= TIOCM_CTS;
> -
> -	if (reg & UARTMODEM_RXRTSE)
> -		temp |= TIOCM_RTS;
> -
> -	return temp;
>From reading the commit log I would expect that you only touch the
set_mctrl function, but not get_mctrl. Assuming your code change is
right, can you mention this in the commit log please? The bits
UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode?

What is the problem you're fixing here? I'm not sure how such an UART
should be handled, but I imagine that you want to make use of automatic
mode in some cases. Greg?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Bhuvanchandra DV]

Hello,

On 06/08/2015 12:11 PM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote:
>> The LPUART does not provide manual control of RTS/CTS signals,
>> those can only be controlled by the hardware directly. Therefore
>> manual control of those signals through mctrl can not be provided.
>> The current implementation enables/disables the automatic control,
>> which is not what mctrl should do, hence remove the incorrect
>> implementation.
>>
>> Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
>> ---
>>   drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------
>>   1 file changed, 7 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
>> index 08ce76f..532cfb7 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
>>
>>   static unsigned int lpuart_get_mctrl(struct uart_port *port)
>>   {
>> -	unsigned int temp = 0;
>> -	unsigned char reg;
>> -
>> -	reg = readb(port->membase + UARTMODEM);
>> -	if (reg & UARTMODEM_TXCTSE)
>> -		temp |= TIOCM_CTS;
>> -
>> -	if (reg & UARTMODEM_RXRTSE)
>> -		temp |= TIOCM_RTS;
>> -
>> -	return temp;
>  From reading the commit log I would expect that you only touch the
> set_mctrl function, but not get_mctrl. Assuming your code change is
> right, can you mention this in the commit log please? The bits
> UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode?
OK, will use 'get/set_mctrl' instead of 'mctrl' in the commit log.
Yes, those bits are only responsible for enabling/disabling the auto 
hardware flow control, not for controlling the RTS/CTS signals directly.
>
> What is the problem you're fixing here? I'm not sure how such an UART
> should be handled, but I imagine that you want to make use of automatic
> mode in some cases. Greg?
>
Fixing the implementation of hardware flow control wrt LPUART hardware.
> Best regards
> Uwe
>

Best regards,
Bhuvan

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Russell King - ARM Linux]

On Mon, Jun 08, 2015 at 08:41:13AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote:
> > The LPUART does not provide manual control of RTS/CTS signals,
> > those can only be controlled by the hardware directly. Therefore
> > manual control of those signals through mctrl can not be provided.
> > The current implementation enables/disables the automatic control,
> > which is not what mctrl should do, hence remove the incorrect
> > implementation.
> > 
> > Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 63 +++++------------------------------------
> >  1 file changed, 7 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> > index 08ce76f..532cfb7 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -822,64 +822,15 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
> >  
> >  static unsigned int lpuart_get_mctrl(struct uart_port *port)
> >  {
> > -	unsigned int temp = 0;
> > -	unsigned char reg;
> > -
> > -	reg = readb(port->membase + UARTMODEM);
> > -	if (reg & UARTMODEM_TXCTSE)
> > -		temp |= TIOCM_CTS;
> > -
> > -	if (reg & UARTMODEM_RXRTSE)
> > -		temp |= TIOCM_RTS;
> > -
> > -	return temp;
> >From reading the commit log I would expect that you only touch the
> set_mctrl function, but not get_mctrl. Assuming your code change is
> right, can you mention this in the commit log please? The bits
> UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode?

Dumb serial writers common fault #1 is to make get_mctrl return TIOCM_RTS
and/or TIOCM_DTR.  It should never return these.  This is a sure sign
that the serial writer doesn't know what they're doing, and doesn't
understand the interface they're implementing.  That's a good enough
reason IMHO to reject a driver prior to merging.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Russell King - ARM Linux]

On Mon, Jun 08, 2015 at 08:41:13AM +0200, Uwe Kleine-K?nig wrote:
> >From reading the commit log I would expect that you only touch the
> set_mctrl function, but not get_mctrl. Assuming your code change is
> right, can you mention this in the commit log please? The bits
> UARTMODEM_TXCTSE and UARTMODEM_RXRTSE only control the automatic mode?

And... another point. UARTMODEM_TXCTSE and UARTMODEM_RXRTSE are the
auto flow control _enable_ signals.  That's not something that
get_mctrl() should ever return.  get_mctrl() is about reporting the
actual current state of the CTS, DSR, DCD and RI signals _only_.

If the hardware doesn't let you read the state, then reporting that
CTS, DSR and DCD are all asserted is recommended, as that's the
state needed to make the UART do useful stuff.  So that part of
this patch is the sane thing to do.

What isn't sane is to use the auto-cts enable bit to report a
ficticious current CTS state.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote:
> +    /*
> +     * CTS/RTS can _only_ be handled
> +     * automatically by the hardware.
> +     */

That's really not acceptable.  You need to do something here so that you
can _force_ RTS to be deasserted when the kernel buffers fill up - be
that by stopping reading characters and letting the FIFO fill up or
something of that ilk.

Ignoring the rest of the flow control system in the kernel is not really
acceptable.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH V2 2/3] tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl

[Bhuvanchandra DV]

On 06/08/2015 02:42 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 01, 2015 at 10:51:06AM +0530, Bhuvanchandra DV wrote:
>> +    /*
>> +     * CTS/RTS can _only_ be handled
>> +     * automatically by the hardware.
>> +     */
>
> That's really not acceptable.  You need to do something here so that you
> can _force_ RTS to be deasserted when the kernel buffers fill up - be
> that by stopping reading characters and letting the FIFO fill up or
> something of that ilk.
Agreed, will fix this.
>
> Ignoring the rest of the flow control system in the kernel is not really
> acceptable.
>

Best regards,
Bhuvan

[PATCH V2 3/3] tty: serial: fsl_lpuart: Add support for RS-485

[Bhuvanchandra DV]

Enable Vybrid's build-in support for RS-485 auto RTS for
controlling line direction of RS-485 transceiver driver.

Signed-off-by: Bhuvanchandra DV <bhuvanchandra.dv@toradex.com>
---
 drivers/tty/serial/fsl_lpuart.c | 56 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 532cfb7..7b4e04a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -820,6 +820,49 @@ static unsigned int lpuart32_tx_empty(struct uart_port *port)
 		TIOCSER_TEMT : 0;
 }
 
+static int lpuart_config_rs485(struct uart_port *port,
+			struct serial_rs485 *rs485)
+{
+	struct lpuart_port *sport = container_of(port,
+			struct lpuart_port, port);
+	u8 modem = readb(sport->port.membase + UARTMODEM) &
+		~(UARTMODEM_TXRTSPOL | UARTMODEM_TXRTSE);
+
+	writeb(modem, sport->port.membase + UARTMODEM);
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		/* Enable auto RS-485 RTS mode */
+		modem |= UARTMODEM_TXRTSE;
+
+		/*
+		 * RTS needs to be logic HIGH either during transer _or_ after
+		 * transfer, other variants are not supported by the hardware.
+		 */
+		if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
+					SER_RS485_RTS_AFTER_SEND)))
+			rs485->flags |= SER_RS485_RTS_ON_SEND;
+
+		if (rs485->flags & SER_RS485_RTS_ON_SEND && rs485->flags &
+					SER_RS485_RTS_AFTER_SEND)
+			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+
+		/*
+		 * The hardware defaults to RTS logic HIGH while transfer.
+		 * Switch polarity in case RTS shall be logic HIGH
+		 * after transfer.
+		 * Note: UART is assumed to be active high.
+		 */
+		if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
+			modem |= UARTMODEM_TXRTSPOL;
+	}
+
+	/* Store the new configuration */
+	sport->port.rs485 = *rs485;
+
+	writeb(modem, sport->port.membase + UARTMODEM);
+	return 0;
+}
+
 static unsigned int lpuart_get_mctrl(struct uart_port *port)
 {
 	return TIOCM_CAR | TIOCM_CTS | TIOCM_DSR;
@@ -1191,6 +1234,12 @@ lpuart_set_termios(struct uart_port *port, struct ktermios *termios,
 		cr1 |= UARTCR1_M;
 	}
 
+	/* When auto RS-485 RTS mode is enabled,
+	 * hardware flow control need to be disabled.
+	 */
+	if (sport->port.rs485.flags & SER_RS485_ENABLED)
+		termios->c_cflag &= ~CRTSCTS;
+
 	if (termios->c_cflag & CRTSCTS) {
 		modem |= (UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
 	} else {
@@ -1749,6 +1798,8 @@ static int lpuart_probe(struct platform_device *pdev)
 		sport->port.ops = &lpuart_pops;
 	sport->port.flags = UPF_BOOT_AUTOCONF;
 
+	sport->port.rs485_config = lpuart_config_rs485;
+
 	sport->clk = devm_clk_get(&pdev->dev, "ipg");
 	if (IS_ERR(sport->clk)) {
 		ret = PTR_ERR(sport->clk);
@@ -1789,6 +1840,11 @@ static int lpuart_probe(struct platform_device *pdev)
 		dev_info(sport->port.dev, "DMA rx channel request failed, "
 				"operating without rx DMA\n");
 
+	if (of_property_read_bool(np, "linux,rs485-enabled-at-boot-time")) {
+		sport->port.rs485.flags |= SER_RS485_ENABLED;
+		sport->port.rs485.flags |= SER_RS485_RTS_ON_SEND;
+		writeb(UARTMODEM_TXRTSE, sport->port.membase + UARTMODEM);
+
 	return 0;
 }
 
-- 
2.1.0

[PATCH V2 0/3] Add RS-485 support and fix RTS/CTS implementation

[Bhuvanchandra DV]

Hello,

Ping!

On 06/01/2015 10:51 AM, Bhuvanchandra DV wrote:
> Changes since V1:
> Use common get/set mctrl control methods for both lpuart, lpuart32
> and fix indention.
>
> Bhuvanchandra DV (3):
>    ARM: dts: colibri-vf: Add pinmux for UART_0 aka UART_A RTS/CTS pins
>    tty: serial: fsl_lpuart: remove RTS/CTS control from set/get_mctrl
>    tty: serial: fsl_lpuart: Add support for RS-485
>
>   arch/arm/boot/dts/vf-colibri.dtsi |   2 +
>   drivers/tty/serial/fsl_lpuart.c   | 103 ++++++++++++++++++++------------------
>   2 files changed, 57 insertions(+), 48 deletions(-)
>

[PATCH V2 0/3] Add RS-485 support and fix RTS/CTS implementation

[Greg KH]

On Mon, Jun 08, 2015 at 11:28:13AM +0530, Bhuvanchandra DV wrote:
> Hello,
> 
> Ping!
> 
> On 06/01/2015 10:51 AM, Bhuvanchandra DV wrote:

7 days later?  Please be patient...