[PATCH] serial: 8250: 8250_omap: Support native RS485

[PATCH] serial: 8250: 8250_omap: Support native RS485

[Lukas Wunner]

Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
automatically assert RTS when data is transmitted, obviating the need
to emulate this functionality in software.

The feature is controlled through new DIR_EN and DIR_POL bits in the
Mode Definition Register 3.  For details see page 8783 and 8890 of the
AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf

Tested-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Su Bao Cheng <baocheng.su@siemens.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Nishanth Menon <nm@ti.com>
---
Matthias tested this patch on the AM64 board TQMa6442L + MBaX4XxL:
He says both sending and receiving continue to work.
He's verified via /dev/mem that DIR_EN and DIR_POL bits are set
on an RS485-enabled UART and are not set on other UARTs.
Also RTS polarity is correct and not changed by the patch.

 drivers/tty/serial/8250/8250_omap.c | 67 ++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 68f5a16..e734efe8 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -44,6 +44,7 @@
 #define	UART_HAS_EFR2			BIT(4)
 #define UART_HAS_RHR_IT_DIS		BIT(5)
 #define UART_RX_TIMEOUT_QUIRK		BIT(6)
+#define UART_HAS_NATIVE_RS485		BIT(7)
 
 #define OMAP_UART_FCR_RX_TRIG		6
 #define OMAP_UART_FCR_TX_TRIG		4
@@ -101,6 +102,11 @@
 #define UART_OMAP_IER2			0x1B
 #define UART_OMAP_IER2_RHR_IT_DIS	BIT(2)
 
+/* Mode Definition Register 3 */
+#define UART_OMAP_MDR3			0x20
+#define UART_OMAP_MDR3_DIR_POL		BIT(3)
+#define UART_OMAP_MDR3_DIR_EN		BIT(4)
+
 /* Enhanced features register 2 */
 #define UART_OMAP_EFR2			0x23
 #define UART_OMAP_EFR2_TIMEOUT_BEHAVE	BIT(6)
@@ -112,6 +118,7 @@ struct omap8250_priv {
 	int line;
 	u8 habit;
 	u8 mdr1;
+	u8 mdr3;
 	u8 efr;
 	u8 scr;
 	u8 wer;
@@ -345,7 +352,9 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 
 	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
 
-	if (up->port.rs485.flags & SER_RS485_ENABLED)
+	if (priv->habit & UART_HAS_NATIVE_RS485)
+		serial_out(up, UART_OMAP_MDR3, priv->mdr3);
+	else if (up->port.rs485.flags & SER_RS485_ENABLED)
 		serial8250_em485_stop_tx(up);
 }
 
@@ -793,6 +802,49 @@ static void omap_8250_unthrottle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+static int omap8250_rs485_config(struct uart_port *port,
+				 struct ktermios *termios,
+				 struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct omap8250_priv *priv = port->private_data;
+	unsigned int baud;
+
+	/*
+	 * There is a fixed delay of 3 bit clock cycles after the TX shift
+	 * register is going empty to allow time for the stop bit to transition
+	 * through the transceiver before direction is changed to receive.
+	 */
+	if (priv->quot) {
+		if (priv->mdr1 & UART_OMAP_MDR1_16X_MODE)
+			baud = port->uartclk / (16 * priv->quot);
+		else
+			baud = port->uartclk / (13 * priv->quot);
+		rs485->delay_rts_after_send = 3 * MSEC_PER_SEC / baud;
+	} else {
+		rs485->delay_rts_after_send = 0;
+	}
+	rs485->delay_rts_before_send = 0;
+
+	if (rs485->flags & SER_RS485_ENABLED)
+		priv->mdr3 |= UART_OMAP_MDR3_DIR_EN;
+	else
+		priv->mdr3 &= ~UART_OMAP_MDR3_DIR_EN;
+
+	/*
+	 * Retain same polarity semantics as RS485 software emulation,
+	 * i.e. SER_RS485_RTS_ON_SEND means driving RTS low on send.
+	 */
+	if (rs485->flags & SER_RS485_RTS_ON_SEND)
+		priv->mdr3 &= ~UART_OMAP_MDR3_DIR_POL;
+	else
+		priv->mdr3 |= UART_OMAP_MDR3_DIR_POL;
+
+	serial_out(up, UART_OMAP_MDR3, priv->mdr3);
+
+	return 0;
+}
+
 #ifdef CONFIG_SERIAL_8250_DMA
 static int omap_8250_rx_dma(struct uart_8250_port *p);
 
@@ -1242,7 +1294,7 @@ static int omap8250_no_handle_irq(struct uart_port *port)
 static struct omap8250_platdata am654_platdata = {
 	.dma_params	= &am654_dma,
 	.habit		= UART_HAS_EFR2 | UART_HAS_RHR_IT_DIS |
-			  UART_RX_TIMEOUT_QUIRK,
+			  UART_RX_TIMEOUT_QUIRK | UART_HAS_NATIVE_RS485,
 };
 
 static struct omap8250_platdata am33xx_platdata = {
@@ -1335,10 +1387,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	up.port.shutdown = omap_8250_shutdown;
 	up.port.throttle = omap_8250_throttle;
 	up.port.unthrottle = omap_8250_unthrottle;
-	up.port.rs485_config = serial8250_em485_config;
 	up.port.rs485_supported = serial8250_em485_supported;
-	up.rs485_start_tx = serial8250_em485_start_tx;
-	up.rs485_stop_tx = serial8250_em485_stop_tx;
 	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
 
 	ret = of_alias_get_id(np, "serial");
@@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
 			 DEFAULT_CLK_SPEED);
 	}
 
+	if (priv->habit & UART_HAS_NATIVE_RS485) {
+		up.port.rs485_config = omap8250_rs485_config;
+	} else {
+		up.port.rs485_config = serial8250_em485_config;
+		up.rs485_start_tx = serial8250_em485_start_tx;
+		up.rs485_stop_tx = serial8250_em485_stop_tx;
+	}
+
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	priv->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	cpu_latency_qos_add_request(&priv->pm_qos_request, priv->latency);
-- 
2.36.1

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Ilpo Järvinen]

On Tue, 27 Sep 2022, Lukas Wunner wrote:

> Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> automatically assert RTS when data is transmitted, obviating the need
> to emulate this functionality in software.
> 
> The feature is controlled through new DIR_EN and DIR_POL bits in the
> Mode Definition Register 3.  For details see page 8783 and 8890 of the
> AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> 
> Tested-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Su Bao Cheng <baocheng.su@siemens.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: Nishanth Menon <nm@ti.com>

> @@ -1335,10 +1387,7 @@ static int omap8250_probe(struct platform_device *pdev)
>  	up.port.shutdown = omap_8250_shutdown;
>  	up.port.throttle = omap_8250_throttle;
>  	up.port.unthrottle = omap_8250_unthrottle;
> -	up.port.rs485_config = serial8250_em485_config;
>  	up.port.rs485_supported = serial8250_em485_supported;
> -	up.rs485_start_tx = serial8250_em485_start_tx;
> -	up.rs485_stop_tx = serial8250_em485_stop_tx;
>  	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
>  
>  	ret = of_alias_get_id(np, "serial");
> @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
>  			 DEFAULT_CLK_SPEED);
>  	}
>  
> +	if (priv->habit & UART_HAS_NATIVE_RS485) {
> +		up.port.rs485_config = omap8250_rs485_config;
> +	} else {
> +		up.port.rs485_config = serial8250_em485_config;
> +		up.rs485_start_tx = serial8250_em485_start_tx;
> +		up.rs485_stop_tx = serial8250_em485_stop_tx;
> +	}

I guess .rs485_supported shouldn't be equal in both cases?

Somehow I feel this hw rs485 + em485 fallback setup might become a common 
pattern so adding some helper for it might be warranted (dwlib already has 
a similar if construct).

-- 
 i.

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Bin Liu]

Hi Lukas,

Thanks for enabling this feature.

On Tue, Sep 27, 2022 at 02:10:01PM +0200, Lukas Wunner wrote:
> Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> automatically assert RTS when data is transmitted, obviating the need
> to emulate this functionality in software.
> 
> The feature is controlled through new DIR_EN and DIR_POL bits in the
> Mode Definition Register 3.  For details see page 8783 and 8890 of the
> AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> 
> Tested-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Su Bao Cheng <baocheng.su@siemens.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: Nishanth Menon <nm@ti.com>
> ---
> Matthias tested this patch on the AM64 board TQMa6442L + MBaX4XxL:
> He says both sending and receiving continue to work.
> He's verified via /dev/mem that DIR_EN and DIR_POL bits are set
> on an RS485-enabled UART and are not set on other UARTs.
> Also RTS polarity is correct and not changed by the patch.
> 
>  drivers/tty/serial/8250/8250_omap.c | 67 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 68f5a16..e734efe8 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -44,6 +44,7 @@
>  #define	UART_HAS_EFR2			BIT(4)
>  #define UART_HAS_RHR_IT_DIS		BIT(5)
>  #define UART_RX_TIMEOUT_QUIRK		BIT(6)
> +#define UART_HAS_NATIVE_RS485		BIT(7)
>  
>  #define OMAP_UART_FCR_RX_TRIG		6
>  #define OMAP_UART_FCR_TX_TRIG		4
> @@ -101,6 +102,11 @@
>  #define UART_OMAP_IER2			0x1B
>  #define UART_OMAP_IER2_RHR_IT_DIS	BIT(2)
>  
> +/* Mode Definition Register 3 */
> +#define UART_OMAP_MDR3			0x20
> +#define UART_OMAP_MDR3_DIR_POL		BIT(3)
> +#define UART_OMAP_MDR3_DIR_EN		BIT(4)
> +
>  /* Enhanced features register 2 */
>  #define UART_OMAP_EFR2			0x23
>  #define UART_OMAP_EFR2_TIMEOUT_BEHAVE	BIT(6)
> @@ -112,6 +118,7 @@ struct omap8250_priv {
>  	int line;
>  	u8 habit;
>  	u8 mdr1;
> +	u8 mdr3;
>  	u8 efr;
>  	u8 scr;
>  	u8 wer;
> @@ -345,7 +352,9 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
>  
>  	up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>  
> -	if (up->port.rs485.flags & SER_RS485_ENABLED)
> +	if (priv->habit & UART_HAS_NATIVE_RS485)
> +		serial_out(up, UART_OMAP_MDR3, priv->mdr3);

This makes the NATIVE_RS485 always used if the SoC supports it, but

> +	else if (up->port.rs485.flags & SER_RS485_ENABLED)
>  		serial8250_em485_stop_tx(up);

there are cases em485 should be used even if SoC supports NATIVE_RS485.
For example:
- the design has pinmux conflict and the RTS pin has to be used for
  something else. Then a GPIO pin would be used for RS485 DE control;
- the design requires customized delay_rts_before_send or
  delay_rts_after_send which NATIVE_RS485 doesn't support;

So we might need an option for such usecases. A device tree flag?

-Bin.

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Lukas Wunner]

On Mon, Oct 03, 2022 at 10:10:59AM -0500, Bin Liu wrote:
> On Tue, Sep 27, 2022 at 02:10:01PM +0200, Lukas Wunner wrote:
> > Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> > automatically assert RTS when data is transmitted, obviating the need
> > to emulate this functionality in software.
> > 
> > The feature is controlled through new DIR_EN and DIR_POL bits in the
> > Mode Definition Register 3.  For details see page 8783 and 8890 of the
> > AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
[...]
> > -	if (up->port.rs485.flags & SER_RS485_ENABLED)
> > +	if (priv->habit & UART_HAS_NATIVE_RS485)
> > +		serial_out(up, UART_OMAP_MDR3, priv->mdr3);
> 
> This makes the NATIVE_RS485 always used if the SoC supports it, but
> 
> > +	else if (up->port.rs485.flags & SER_RS485_ENABLED)
> >  		serial8250_em485_stop_tx(up);
> 
> there are cases em485 should be used even if SoC supports NATIVE_RS485.
> For example:
> - the design has pinmux conflict and the RTS pin has to be used for
>   something else. Then a GPIO pin would be used for RS485 DE control;
> - the design requires customized delay_rts_before_send or
>   delay_rts_after_send which NATIVE_RS485 doesn't support;
> 
> So we might need an option for such usecases. A device tree flag?

I expect those cases to be rare, hence do not see the need to
address them right from the start.  Support for falling back
to software emulation can easily be added later.  Indeed a
device tree property might be an appropriate way to trigger
such a fallback.

Thanks,

Lukas

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Lukas Wunner]

On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
> On Tue, 27 Sep 2022, Lukas Wunner wrote:
> > Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> > automatically assert RTS when data is transmitted, obviating the need
> > to emulate this functionality in software.
> > 
> > The feature is controlled through new DIR_EN and DIR_POL bits in the
> > Mode Definition Register 3.  For details see page 8783 and 8890 of the
> > AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
[...]
> > @@ -1335,10 +1387,7 @@ static int omap8250_probe(struct platform_device *pdev)
> >  	up.port.shutdown = omap_8250_shutdown;
> >  	up.port.throttle = omap_8250_throttle;
> >  	up.port.unthrottle = omap_8250_unthrottle;
> > -	up.port.rs485_config = serial8250_em485_config;
> >  	up.port.rs485_supported = serial8250_em485_supported;
> > -	up.rs485_start_tx = serial8250_em485_start_tx;
> > -	up.rs485_stop_tx = serial8250_em485_stop_tx;
> >  	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
> >  
> >  	ret = of_alias_get_id(np, "serial");
> > @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
> >  			 DEFAULT_CLK_SPEED);
> >  	}
> >  
> > +	if (priv->habit & UART_HAS_NATIVE_RS485) {
> > +		up.port.rs485_config = omap8250_rs485_config;
> > +	} else {
> > +		up.port.rs485_config = serial8250_em485_config;
> > +		up.rs485_start_tx = serial8250_em485_start_tx;
> > +		up.rs485_stop_tx = serial8250_em485_stop_tx;
> > +	}
> 
> I guess .rs485_supported shouldn't be equal in both cases?

I contemplated whether it should be different for hardware-assisted
RS485 but came to the conclusion that it shouldn't:

The polarity may be chosen both for hardware- and software-controlled RTS.

Whether RX_DURING_TX is possible depends on how the RS485 transceiver
is wired to the UART:  If RTS asserts !RE on the transceiver when sending,
the UART cannot receive data, regardless whether hardware- or software-
controlled RTS is used.

TERMINATE_BUS works independently from RTS control.

And ADDRB doesn't seem to be supported in either mode AFAICS.

Am I missing something?

Thanks,

Lukas

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]

On Mon, 3 Oct 2022, Lukas Wunner wrote:

> On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
> > On Tue, 27 Sep 2022, Lukas Wunner wrote:
> > > Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> > > automatically assert RTS when data is transmitted, obviating the need
> > > to emulate this functionality in software.
> > > 
> > > The feature is controlled through new DIR_EN and DIR_POL bits in the
> > > Mode Definition Register 3.  For details see page 8783 and 8890 of the
> > > AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> [...]
> > > @@ -1335,10 +1387,7 @@ static int omap8250_probe(struct platform_device *pdev)
> > >  	up.port.shutdown = omap_8250_shutdown;
> > >  	up.port.throttle = omap_8250_throttle;
> > >  	up.port.unthrottle = omap_8250_unthrottle;
> > > -	up.port.rs485_config = serial8250_em485_config;
> > >  	up.port.rs485_supported = serial8250_em485_supported;
> > > -	up.rs485_start_tx = serial8250_em485_start_tx;
> > > -	up.rs485_stop_tx = serial8250_em485_stop_tx;
> > >  	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
> > >  
> > >  	ret = of_alias_get_id(np, "serial");
> > > @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
> > >  			 DEFAULT_CLK_SPEED);
> > >  	}
> > >  
> > > +	if (priv->habit & UART_HAS_NATIVE_RS485) {
> > > +		up.port.rs485_config = omap8250_rs485_config;
> > > +	} else {
> > > +		up.port.rs485_config = serial8250_em485_config;
> > > +		up.rs485_start_tx = serial8250_em485_start_tx;
> > > +		up.rs485_stop_tx = serial8250_em485_stop_tx;
> > > +	}
> > 
> > I guess .rs485_supported shouldn't be equal in both cases?
> 
> I contemplated whether it should be different for hardware-assisted
> RS485 but came to the conclusion that it shouldn't:
> 
> The polarity may be chosen both for hardware- and software-controlled RTS.
> 
> Whether RX_DURING_TX is possible depends on how the RS485 transceiver
> is wired to the UART:  If RTS asserts !RE on the transceiver when sending,
> the UART cannot receive data, regardless whether hardware- or software-
> controlled RTS is used.
> 
> TERMINATE_BUS works independently from RTS control.
> 
> And ADDRB doesn't seem to be supported in either mode AFAICS.
> 
> Am I missing something?

Core is not handling just flags but also delay_rts_before_send and 
delay_rts_after_send sanitization. See 
uart_sanitize_serial_rs485_delays().

Btw, you can also get rid of this line once you provide separate 
rs485_supported:
	rs485->delay_rts_before_send = 0;

What to do with delay_rts_after_send seems bit trickier though. Looking 
the code, it cannot be configured to arbitrary values by the user but it 
might not be zero either after the driver touches it. Maybe it safer to 
have it supported (set to 1) to avoid spuriously triggering the warning in 
uart_sanitize_serial_rs485_delays() (e.g., during init if non-zero delay 
is provided).

-- 
 i.

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Bin Liu]

Hi,

On Mon, Oct 03, 2022 at 09:42:24PM +0200, Lukas Wunner wrote:
> On Mon, Oct 03, 2022 at 10:10:59AM -0500, Bin Liu wrote:
> > On Tue, Sep 27, 2022 at 02:10:01PM +0200, Lukas Wunner wrote:
> > > Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> > > automatically assert RTS when data is transmitted, obviating the need
> > > to emulate this functionality in software.
> > > 
> > > The feature is controlled through new DIR_EN and DIR_POL bits in the
> > > Mode Definition Register 3.  For details see page 8783 and 8890 of the
> > > AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> [...]
> > > -	if (up->port.rs485.flags & SER_RS485_ENABLED)
> > > +	if (priv->habit & UART_HAS_NATIVE_RS485)
> > > +		serial_out(up, UART_OMAP_MDR3, priv->mdr3);
> > 
> > This makes the NATIVE_RS485 always used if the SoC supports it, but
> > 
> > > +	else if (up->port.rs485.flags & SER_RS485_ENABLED)
> > >  		serial8250_em485_stop_tx(up);
> > 
> > there are cases em485 should be used even if SoC supports NATIVE_RS485.
> > For example:
> > - the design has pinmux conflict and the RTS pin has to be used for
> >   something else. Then a GPIO pin would be used for RS485 DE control;
> > - the design requires customized delay_rts_before_send or
> >   delay_rts_after_send which NATIVE_RS485 doesn't support;
> > 
> > So we might need an option for such usecases. A device tree flag?
> 
> I expect those cases to be rare, hence do not see the need to

Maybe rare, but I know some projects use GPIO for DE control.

> address them right from the start.  Support for falling back

So I think missing it is a regression, because this patch forces to
use the RTS pin for DE control, this breaks the existing projects which
use GPIO for RTS/DE or have customized delay_rts_{before,after}_send.

-Bin.

> to software emulation can easily be added later.  Indeed a
> device tree property might be an appropriate way to trigger
> such a fallback.
> 
> Thanks,
> 
> Lukas

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Lukas Wunner]

On Tue, Oct 04, 2022 at 12:06:21PM +0300, Ilpo Järvinen wrote:
> On Mon, 3 Oct 2022, Lukas Wunner wrote:
> > On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
> > > On Tue, 27 Sep 2022, Lukas Wunner wrote:
> > > > @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
> > > >  			 DEFAULT_CLK_SPEED);
> > > >  	}
> > > >  
> > > > +	if (priv->habit & UART_HAS_NATIVE_RS485) {
> > > > +		up.port.rs485_config = omap8250_rs485_config;
> > > > +	} else {
> > > > +		up.port.rs485_config = serial8250_em485_config;
> > > > +		up.rs485_start_tx = serial8250_em485_start_tx;
> > > > +		up.rs485_stop_tx = serial8250_em485_stop_tx;
> > > > +	}
> > > 
> > > I guess .rs485_supported shouldn't be equal in both cases?
> > 
> > I contemplated whether it should be different for hardware-assisted
> > RS485 but came to the conclusion that it shouldn't:
[...]
> Core is not handling just flags but also delay_rts_before_send and 
> delay_rts_after_send sanitization. See 
> uart_sanitize_serial_rs485_delays().
> 
> Btw, you can also get rid of this line once you provide separate 
> rs485_supported:
> 	rs485->delay_rts_before_send = 0;
> 
> What to do with delay_rts_after_send seems bit trickier though. Looking 
> the code, it cannot be configured to arbitrary values by the user but it 
> might not be zero either after the driver touches it. Maybe it safer to 
> have it supported (set to 1) to avoid spuriously triggering the warning in 
> uart_sanitize_serial_rs485_delays() (e.g., during init if non-zero delay 
> is provided).

If I understand Figure 12-276 on page 8783 of the AM65 TRM correctly,
there appears to be a 1 bit clock delay between writing to the THR register
and transmission of the start bit:

  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf

I intend to respin the patch with the following addition:

  fixed_delay_rts_before_send = 1 * MSEC_PER_SEC / baud;

As a result, both delay_rts_before_send and delay_rts_after_send should be
set to 1 in the rs485_supported struct for hardware-controlled RTS.

The resulting struct is identical to serial8250_em485_supported.

Thanks,

Lukas

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Vignesh Raghavendra]

Hi,

On 04/10/22 7:15 pm, Bin Liu wrote:
> Hi,
> 
> On Mon, Oct 03, 2022 at 09:42:24PM +0200, Lukas Wunner wrote:
>> On Mon, Oct 03, 2022 at 10:10:59AM -0500, Bin Liu wrote:
>>> On Tue, Sep 27, 2022 at 02:10:01PM +0200, Lukas Wunner wrote:
>>>> Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
>>>> automatically assert RTS when data is transmitted, obviating the need
>>>> to emulate this functionality in software.
>>>>
>>>> The feature is controlled through new DIR_EN and DIR_POL bits in the
>>>> Mode Definition Register 3.  For details see page 8783 and 8890 of the
>>>> AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>> [...]
>>>> -	if (up->port.rs485.flags & SER_RS485_ENABLED)
>>>> +	if (priv->habit & UART_HAS_NATIVE_RS485)
>>>> +		serial_out(up, UART_OMAP_MDR3, priv->mdr3);
>>>
>>> This makes the NATIVE_RS485 always used if the SoC supports it, but
>>>
>>>> +	else if (up->port.rs485.flags & SER_RS485_ENABLED)
>>>>  		serial8250_em485_stop_tx(up);
>>>
>>> there are cases em485 should be used even if SoC supports NATIVE_RS485.
>>> For example:
>>> - the design has pinmux conflict and the RTS pin has to be used for
>>>   something else. Then a GPIO pin would be used for RS485 DE control;
>>> - the design requires customized delay_rts_before_send or
>>>   delay_rts_after_send which NATIVE_RS485 doesn't support;
>>>
>>> So we might need an option for such usecases. A device tree flag?
>>
>> I expect those cases to be rare, hence do not see the need to
> 
> Maybe rare, but I know some projects use GPIO for DE control.
> 
>> address them right from the start.  Support for falling back
> 
> So I think missing it is a regression, because this patch forces to
> use the RTS pin for DE control, this breaks the existing projects which
> use GPIO for RTS/DE or have customized delay_rts_{before,after}_send.

I agree with Bin. This patch as such can break DTs which intend to use
GPIO based DE.
Quick fix would be to simply look for presence of rts-gpios property in
DT, if so fallback to registering serial8250_em485_*. This should avoid
regressing DTs using GPIO for DE control.

Regards
Vignesh

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Vignesh Raghavendra]

On 06/10/22 11:51 am, Lukas Wunner wrote:
> On Tue, Oct 04, 2022 at 12:06:21PM +0300, Ilpo Järvinen wrote:
>> On Mon, 3 Oct 2022, Lukas Wunner wrote:
>>> On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
>>>> On Tue, 27 Sep 2022, Lukas Wunner wrote:
>>>>> @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
>>>>>  			 DEFAULT_CLK_SPEED);
>>>>>  	}
>>>>>  
>>>>> +	if (priv->habit & UART_HAS_NATIVE_RS485) {
>>>>> +		up.port.rs485_config = omap8250_rs485_config;
>>>>> +	} else {
>>>>> +		up.port.rs485_config = serial8250_em485_config;
>>>>> +		up.rs485_start_tx = serial8250_em485_start_tx;
>>>>> +		up.rs485_stop_tx = serial8250_em485_stop_tx;
>>>>> +	}
>>>>
>>>> I guess .rs485_supported shouldn't be equal in both cases?
>>>
>>> I contemplated whether it should be different for hardware-assisted
>>> RS485 but came to the conclusion that it shouldn't:
> [...]
>> Core is not handling just flags but also delay_rts_before_send and 
>> delay_rts_after_send sanitization. See 
>> uart_sanitize_serial_rs485_delays().
>>
>> Btw, you can also get rid of this line once you provide separate 
>> rs485_supported:
>> 	rs485->delay_rts_before_send = 0;
>>
>> What to do with delay_rts_after_send seems bit trickier though. Looking 
>> the code, it cannot be configured to arbitrary values by the user but it 
>> might not be zero either after the driver touches it. Maybe it safer to 
>> have it supported (set to 1) to avoid spuriously triggering the warning in 
>> uart_sanitize_serial_rs485_delays() (e.g., during init if non-zero delay 
>> is provided).
> 
> If I understand Figure 12-276 on page 8783 of the AM65 TRM correctly,
> there appears to be a 1 bit clock delay between writing to the THR register
> and transmission of the start bit:
> 
>   https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> 
> I intend to respin the patch with the following addition:
> 
>   fixed_delay_rts_before_send = 1 * MSEC_PER_SEC / baud;
> 
> As a result, both delay_rts_before_send and delay_rts_after_send should be
> set to 1 in the rs485_supported struct for hardware-controlled RTS.
> 

12.1.5.4.8.2.1 RS-485 External Transceiver Direction Control

The direction is determined by the hardware monitoring the TX FIFO and

the TX shift register. When both are empty the transceiver is set to RX.
There is a guard band delay

counter of **3 bit clock cycles** after the TX shift register is going
empty to allow time for the stop bit to

transition through the transceiver before a direction change to receive
might be applied.

So, RTS deasserts 3 cycle after stop bit. Shouldn't this be 3 baud cycles?


Really appreciate working on this feature, thanks!

Regards
Vignesh

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Lukas Wunner]

On Thu, Oct 06, 2022 at 12:37:43PM +0530, Vignesh Raghavendra wrote:
> On 04/10/22 7:15 pm, Bin Liu wrote:
> > On Mon, Oct 03, 2022 at 09:42:24PM +0200, Lukas Wunner wrote:
> >> On Mon, Oct 03, 2022 at 10:10:59AM -0500, Bin Liu wrote:
> >>> On Tue, Sep 27, 2022 at 02:10:01PM +0200, Lukas Wunner wrote:
> >>>> Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
> >>>> automatically assert RTS when data is transmitted, obviating the need
> >>>> to emulate this functionality in software.
> >>>>
> >>>> The feature is controlled through new DIR_EN and DIR_POL bits in the
> >>>> Mode Definition Register 3.  For details see page 8783 and 8890 of the
> >>>> AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> >> [...]
> >>>> -	if (up->port.rs485.flags & SER_RS485_ENABLED)
> >>>> +	if (priv->habit & UART_HAS_NATIVE_RS485)
> >>>> +		serial_out(up, UART_OMAP_MDR3, priv->mdr3);
> >>>
> >>> This makes the NATIVE_RS485 always used if the SoC supports it, but
> >>>
> >>>> +	else if (up->port.rs485.flags & SER_RS485_ENABLED)
> >>>>  		serial8250_em485_stop_tx(up);
> >>>
> >>> there are cases em485 should be used even if SoC supports NATIVE_RS485.
> >>> For example:
> >>> - the design has pinmux conflict and the RTS pin has to be used for
> >>>   something else. Then a GPIO pin would be used for RS485 DE control;
> >>> - the design requires customized delay_rts_before_send or
> >>>   delay_rts_after_send which NATIVE_RS485 doesn't support;
> >>>
> >>> So we might need an option for such usecases. A device tree flag?
> >>
> >> I expect those cases to be rare, hence do not see the need to
> > 
> > Maybe rare, but I know some projects use GPIO for DE control.
> > 
> >> address them right from the start.  Support for falling back
> > 
> > So I think missing it is a regression, because this patch forces to
> > use the RTS pin for DE control, this breaks the existing projects which
> > use GPIO for RTS/DE or have customized delay_rts_{before,after}_send.
> 
> I agree with Bin. This patch as such can break DTs which intend to use
> GPIO based DE.
> Quick fix would be to simply look for presence of rts-gpios property in
> DT, if so fallback to registering serial8250_em485_*. This should avoid
> regressing DTs using GPIO for DE control.

I've updated the patch to check for

  mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)

as well as user-requested delays that exceed the fixed hardware delays.
The driver falls back to software emulation in that case.

Updated patch is on this development branch and is currently being
compile-tested by Intel's 0-day bot:

https://github.com/l1k/linux/commits/rs485_sitara

Thanks,

Lukas

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Vignesh Raghavendra]

On 06/10/22 12:46 pm, Vignesh Raghavendra wrote:
> 
> 
> On 06/10/22 11:51 am, Lukas Wunner wrote:
>> On Tue, Oct 04, 2022 at 12:06:21PM +0300, Ilpo Järvinen wrote:
>>> On Mon, 3 Oct 2022, Lukas Wunner wrote:
>>>> On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
>>>>> On Tue, 27 Sep 2022, Lukas Wunner wrote:
>>>>>> @@ -1377,6 +1426,14 @@ static int omap8250_probe(struct platform_device *pdev)
>>>>>>  			 DEFAULT_CLK_SPEED);
>>>>>>  	}
>>>>>>  
>>>>>> +	if (priv->habit & UART_HAS_NATIVE_RS485) {
>>>>>> +		up.port.rs485_config = omap8250_rs485_config;
>>>>>> +	} else {
>>>>>> +		up.port.rs485_config = serial8250_em485_config;
>>>>>> +		up.rs485_start_tx = serial8250_em485_start_tx;
>>>>>> +		up.rs485_stop_tx = serial8250_em485_stop_tx;
>>>>>> +	}
>>>>>
>>>>> I guess .rs485_supported shouldn't be equal in both cases?
>>>>
>>>> I contemplated whether it should be different for hardware-assisted
>>>> RS485 but came to the conclusion that it shouldn't:
>> [...]
>>> Core is not handling just flags but also delay_rts_before_send and 
>>> delay_rts_after_send sanitization. See 
>>> uart_sanitize_serial_rs485_delays().
>>>
>>> Btw, you can also get rid of this line once you provide separate 
>>> rs485_supported:
>>> 	rs485->delay_rts_before_send = 0;
>>>
>>> What to do with delay_rts_after_send seems bit trickier though. Looking 
>>> the code, it cannot be configured to arbitrary values by the user but it 
>>> might not be zero either after the driver touches it. Maybe it safer to 
>>> have it supported (set to 1) to avoid spuriously triggering the warning in 
>>> uart_sanitize_serial_rs485_delays() (e.g., during init if non-zero delay 
>>> is provided).
>>
>> If I understand Figure 12-276 on page 8783 of the AM65 TRM correctly,
>> there appears to be a 1 bit clock delay between writing to the THR register
>> and transmission of the start bit:
>>
>>   https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
>>
>> I intend to respin the patch with the following addition:
>>
>>   fixed_delay_rts_before_send = 1 * MSEC_PER_SEC / baud;
>>
>> As a result, both delay_rts_before_send and delay_rts_after_send should be
>> set to 1 in the rs485_supported struct for hardware-controlled RTS.
>>
> 
> 12.1.5.4.8.2.1 RS-485 External Transceiver Direction Control
> 
> The direction is determined by the hardware monitoring the TX FIFO and
> 
> the TX shift register. When both are empty the transceiver is set to RX.
> There is a guard band delay
> 
> counter of **3 bit clock cycles** after the TX shift register is going
> empty to allow time for the stop bit to
> 
> transition through the transceiver before a direction change to receive
> might be applied.
> 
> So, RTS deasserts 3 cycle after stop bit. Shouldn't this be 3 baud cycles?
> 

Oops 3 cycles is for delay_rts_after_send..  1 cycle for
delay_rts_before_send sounds good to me. sorry for the noise

Regards
Vignesh

Re: [PATCH] serial: 8250: 8250_omap: Support native RS485

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

On Thu, 6 Oct 2022, Lukas Wunner wrote:

> On Tue, Oct 04, 2022 at 12:06:21PM +0300, Ilpo Järvinen wrote:
> > On Mon, 3 Oct 2022, Lukas Wunner wrote:
> > > On Wed, Sep 28, 2022 at 02:38:40PM +0300, Ilpo Järvinen wrote:
> > > > 
> > > > I guess .rs485_supported shouldn't be equal in both cases?
> > > 
> > > I contemplated whether it should be different for hardware-assisted
> > > RS485 but came to the conclusion that it shouldn't:
> [...]
> > Core is not handling just flags but also delay_rts_before_send and 
> > delay_rts_after_send sanitization. See 
> > uart_sanitize_serial_rs485_delays().
> > 
> > Btw, you can also get rid of this line once you provide separate 
> > rs485_supported:
> > 	rs485->delay_rts_before_send = 0;
> > 
> > What to do with delay_rts_after_send seems bit trickier though. Looking 
> > the code, it cannot be configured to arbitrary values by the user but it 
> > might not be zero either after the driver touches it. Maybe it safer to 
> > have it supported (set to 1) to avoid spuriously triggering the warning in 
> > uart_sanitize_serial_rs485_delays() (e.g., during init if non-zero delay 
> > is provided).
> 
> If I understand Figure 12-276 on page 8783 of the AM65 TRM correctly,
> there appears to be a 1 bit clock delay between writing to the THR register
> and transmission of the start bit:
> 
>   https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> 
> I intend to respin the patch with the following addition:
> 
>   fixed_delay_rts_before_send = 1 * MSEC_PER_SEC / baud;
> 
> As a result, both delay_rts_before_send and delay_rts_after_send should be
> set to 1 in the rs485_supported struct for hardware-controlled RTS.

Ok. I was just commenting what the code did but of course if you change 
it the situation is different then.

> The resulting struct is identical to serial8250_em485_supported.

Please note it down somewhere (either commit message or comment at the
rs485_supported assignment) so that somebody else after us doesn't need 
to dig up this thread.


-- 
 i.