This series tries to revive the work of Giulio Benetti from 2018 [0]
which seemed to have stalled at that time.
The board I needed that on also had the additional caveat that it
uses non-standard pins for DE/RE so needed gpio mctrl layer as well
and even more special needed to control the RE pin manually not as
part of it being connected to the DE signal as seems to be the standard.
Depends on Lukas Wunner's series adding the rs485 bus term gpio:
https://lore.kernel.org/r/[email protected]
changes in v3:
- wording fixes for dt-binding (Andy)
- maxItems 1 in dt-binding (Lukas)
- use devm_gpiod_get_optional (Andy)
- remove unneeded gpio checks (Andy)
- use GPIOD_OUT_HIGH for initial value (Lukas)
- split loop-reading into the existing timer infrastructure
Changes in v2:
- move to recent rs485 improvements in tty-next
- added a capability for TEMT interrupt presence
- external gpio for optional receiver-enable handling
- timeout polling via the generic readx_poll_timeout
Changes from the 2018 submission include:
- add timeout when waiting for fifos to clear using a new helper
- move on-boot enablement of the rs485 mode to after registering
the port. This saves having to copy the em485 struct as done
originally, which also ran into spinlock-debug warnings when testing
and also makes it actually possible to use the mctrl gpio layer
for non-standard gpios.
Giulio Benetti (2):
serial: 8250: Handle implementations not having TEMT interrupt using
em485
serial: 8250_dw: add em485 support
Heiko Stuebner (3):
serial: 8520_port: Fix function param documentation
dt-bindings: serial: Add binding for rs485 receiver enable GPIO
serial: 8250: Support separate rs485 rx-enable GPIO
.../devicetree/bindings/serial/rs485.yaml | 4 ++
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_bcm2835aux.c | 2 +-
drivers/tty/serial/8250/8250_dw.c | 3 +
drivers/tty/serial/8250/8250_of.c | 2 +
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 60 ++++++++++++++++---
drivers/tty/serial/serial_core.c | 10 ++++
include/linux/serial_core.h | 1 +
9 files changed, 76 insertions(+), 9 deletions(-)
--
2.25.1
From: Heiko Stuebner <[email protected]>
RS485 has two signals to control transmissions "driver enable" (DE) and
"receiver enable" (RE). DE is already handled via the uarts RTS signal
while the RE signal on most implementations doesn't get handled
separately at all.
As there still will be cases where this is needed though add a gpio
property for declaring this signal pin.
Signed-off-by: Heiko Stuebner <[email protected]>
---
Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index a9ad17864889..c61226c235f0 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -44,6 +44,10 @@ properties:
description: enables the receiving of data even while sending data.
$ref: /schemas/types.yaml#/definitions/flag
+ rs485-rx-enable-gpios:
+ description: GPIO to handle a separate RS485 receive enable signal
+ maxItems: 1
+
rs485-term-gpios:
description: GPIO pin to enable RS485 bus termination.
maxItems: 1
--
2.25.1
From: Giulio Benetti <[email protected]>
Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
standard, instead only available on some implementations.
The current em485 implementation does not work on ports without it.
The only chance to make it work is to loop-read on LSR register.
So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
update all current em485 users with that capability and add
a loop-reading during __stop_tx_rs485() on uarts not having it.
As __stop_tx_rs485() can also be called from a hard-irq context the
loop-reading is split. If the fifo clears in under 100us in
__stop_tx_rs485() itself just the regular stop calls get executed.
If it takes longer, re-use the existing stop-timer infrastructure
but with only a 10us timer to again poll the LSR registers.
Signed-off-by: Giulio Benetti <[email protected]>
[moved to use added UART_CAP_TEMT, use readx_poll_timeout]
Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_bcm2835aux.c | 2 +-
drivers/tty/serial/8250/8250_of.c | 2 +
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 51 ++++++++++++++++++++---
5 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 0df02c055107..50c88dd3f857 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -82,6 +82,7 @@ struct serial8250_config {
#define UART_CAP_MINI (1 << 17) /* Mini UART on BCM283X family lacks:
* STOP PARITY EPAR SPAR WLEN5 WLEN6
*/
+#define UART_CAP_TEMT (1 << 18) /* UART has TEMT interrupt */
#define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */
#define UART_BUG_TXEN (1 << 1) /* UART has buggy TX IIR status */
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index 1cc0620b596c..c2226f1fd6ac 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
return -ENOMEM;
/* initialize data */
- up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
+ up.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
up.port.dev = &pdev->dev;
up.port.regshift = 2;
up.port.type = PORT_16550;
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 0b89954a3781..e96abd51454c 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -223,6 +223,8 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
&port8250.overrun_backoff_time_ms) != 0)
port8250.overrun_backoff_time_ms = 0;
+ port8250.capabilities |= UART_CAP_TEMT;
+
ret = serial8250_register_8250_port(&port8250);
if (ret < 0)
goto err_dispose;
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 6cae3782e5fa..241d7307c38f 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1144,7 +1144,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.regshift = 2;
up.port.fifosize = 64;
up.tx_loadsz = 64;
- up.capabilities = UART_CAP_FIFO;
+ up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
#ifdef CONFIG_PM
/*
* Runtime PM is mostly transparent. However to do it right we need to a
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 9e8fec85d1a3..a456e81d3e0b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -19,6 +19,7 @@
#include <linux/moduleparam.h>
#include <linux/ioport.h>
#include <linux/init.h>
+#include <linux/iopoll.h>
#include <linux/console.h>
#include <linux/sysrq.h>
#include <linux/delay.h>
@@ -1467,11 +1468,26 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
+static inline int __get_lsr(struct uart_8250_port *p)
+{
+ return serial_in(p, UART_LSR);
+}
+
+static inline int __wait_for_empty(struct uart_8250_port *p, u64 timeout_us)
+{
+ int lsr;
+
+ return readx_poll_timeout(__get_lsr, p, lsr,
+ (lsr & BOTH_EMPTY) == BOTH_EMPTY,
+ 0, timeout_us);
+}
+
static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
{
struct uart_8250_em485 *em485;
struct uart_8250_port *p;
unsigned long flags;
+ enum hrtimer_restart restart = HRTIMER_NORESTART;
em485 = container_of(t, struct uart_8250_em485, stop_tx_timer);
p = em485->port;
@@ -1479,13 +1495,27 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
serial8250_rpm_get(p);
spin_lock_irqsave(&p->port.lock, flags);
if (em485->active_timer == &em485->stop_tx_timer) {
+ /*
+ * On 8250 without TEMT interrupt, check LSR state and
+ * restart timer if not empty yet.
+ */
+ if (!(p->capabilities & UART_CAP_TEMT)) {
+ int ret = __wait_for_empty(p, 100);
+
+ if (ret < 0) {
+ restart = HRTIMER_RESTART;
+ goto out;
+ }
+ }
+
p->rs485_stop_tx(p);
em485->active_timer = NULL;
em485->tx_stopped = true;
}
+out:
spin_unlock_irqrestore(&p->port.lock, flags);
serial8250_rpm_put(p);
- return HRTIMER_NORESTART;
+ return restart;
}
static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
@@ -1509,6 +1539,13 @@ static void __stop_tx_rs485(struct uart_8250_port *p)
em485->active_timer = &em485->stop_tx_timer;
start_hrtimer_ms(&em485->stop_tx_timer,
p->port.rs485.delay_rts_after_send);
+ } else if (!(p->capabilities & UART_CAP_TEMT) &&
+ __wait_for_empty(p, 100)) {
+ /* Short timer of 1us to check for clear fifos */
+ ktime_t tim = ktime_set(0, 1000);
+
+ em485->active_timer = &em485->stop_tx_timer;
+ hrtimer_start(&em485->stop_tx_timer, tim, HRTIMER_MODE_REL);
} else {
p->rs485_stop_tx(p);
em485->active_timer = NULL;
@@ -1531,11 +1568,15 @@ static inline void __stop_tx(struct uart_8250_port *p)
/*
* To provide required timeing and allow FIFO transfer,
* __stop_tx_rs485() must be called only when both FIFO and
- * shift register are empty. It is for device driver to enable
- * interrupt on TEMT.
+ * shift register are empty. If 8250 port supports it,
+ * it is for device driver to enable interrupt on TEMT.
+ * Otherwise must loop-read until TEMT and THRE flags are set,
+ * which happens in __stop_tx_rs485()
*/
- if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
- return;
+ if (p->capabilities & UART_CAP_TEMT) {
+ if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
+ return;
+ }
__stop_tx_rs485(p);
}
--
2.25.1
From: Heiko Stuebner <[email protected]>
The parameter is named p while the documentation talks about up.
Fix the doc to be in line with the code.
Fixes: 058bc104f7ca ("serial: 8250: Generalize rs485 software emulation")
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/tty/serial/8250/8250_port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 9c0457e74d21..6975bd3ecb7d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1437,7 +1437,7 @@ static void serial8250_stop_rx(struct uart_port *port)
/**
* serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
- * @up: uart 8250 port
+ * @p: uart 8250 port
*
* Generic callback usable by 8250 uart drivers to stop rs485 transmission.
*/
--
2.25.1
On Sun, May 17, 2020 at 11:56:06PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <[email protected]>
>
> The parameter is named p while the documentation talks about up.
> Fix the doc to be in line with the code.
>
Reviewed-by: Andy Shevchenko <[email protected]>
> Fixes: 058bc104f7ca ("serial: 8250: Generalize rs485 software emulation")
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Heiko Stuebner <[email protected]>
> ---
> drivers/tty/serial/8250/8250_port.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 9c0457e74d21..6975bd3ecb7d 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1437,7 +1437,7 @@ static void serial8250_stop_rx(struct uart_port *port)
>
> /**
> * serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
> - * @up: uart 8250 port
> + * @p: uart 8250 port
> *
> * Generic callback usable by 8250 uart drivers to stop rs485 transmission.
> */
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On Sun, May 17, 2020 at 11:56:09PM +0200, Heiko Stuebner wrote:
> From: Giulio Benetti <[email protected]>
>
> Some 8250 ports have a TEMT interrupt but it's not a part of the 8250
> standard, instead only available on some implementations.
>
> The current em485 implementation does not work on ports without it.
> The only chance to make it work is to loop-read on LSR register.
>
> So add UART_CAP_TEMT to mark 8250 uarts having this interrupt,
> update all current em485 users with that capability and add
> a loop-reading during __stop_tx_rs485() on uarts not having it.
>
> As __stop_tx_rs485() can also be called from a hard-irq context the
> loop-reading is split. If the fifo clears in under 100us in
> __stop_tx_rs485() itself just the regular stop calls get executed.
> If it takes longer, re-use the existing stop-timer infrastructure
> but with only a 10us timer to again poll the LSR registers.
>
> Signed-off-by: Giulio Benetti <[email protected]>
> [moved to use added UART_CAP_TEMT, use readx_poll_timeout]
I can't parse first part...
Also, shouldn't it be rather like
[heiko: ...]
?
...
> +static inline int __get_lsr(struct uart_8250_port *p)
> +{
> + return serial_in(p, UART_LSR);
> +}
> +
> +static inline int __wait_for_empty(struct uart_8250_port *p, u64 timeout_us)
> +{
> + int lsr;
> +
> + return readx_poll_timeout(__get_lsr, p, lsr,
> + (lsr & BOTH_EMPTY) == BOTH_EMPTY,
> + 0, timeout_us);
> +}
...
> + int ret = __wait_for_empty(p, 100);
Do you expect something different than 100? If no, perhaps for now just put it
inside the function as a constant?
> + if (ret < 0) {
> + restart = HRTIMER_RESTART;
> + goto out;
> + }
...
> + } else if (!(p->capabilities & UART_CAP_TEMT) &&
> + __wait_for_empty(p, 100)) {
I would leave it on one line even if you leave 100 as a parameter, but it's up to you.
...
> + if (p->capabilities & UART_CAP_TEMT) {
> + if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
> + return;
> + }
if (a) {
if (b) {
...
}
}
is equivalent to
if (a && b) {
...
}
But it's up to you which one to choose.
--
With Best Regards,
Andy Shevchenko
On Sun, May 17, 2020 at 11:56:05PM +0200, Heiko Stuebner wrote:
> This series tries to revive the work of Giulio Benetti from 2018 [0]
> which seemed to have stalled at that time.
>
> The board I needed that on also had the additional caveat that it
> uses non-standard pins for DE/RE so needed gpio mctrl layer as well
> and even more special needed to control the RE pin manually not as
> part of it being connected to the DE signal as seems to be the standard.
[...]
> Giulio Benetti (2):
> serial: 8250: Handle implementations not having TEMT interrupt using
> em485
> serial: 8250_dw: add em485 support
>
> Heiko Stuebner (3):
> serial: 8520_port: Fix function param documentation
> dt-bindings: serial: Add binding for rs485 receiver enable GPIO
> serial: 8250: Support separate rs485 rx-enable GPIO
ICYMI, patch [1/5] of this series got accepted back in the day
and patches [4/5] and [5/5] appeared in slightly modified form in v5.19
(commits b54f7a922d33 and 5ff33917faca).
So only patches [2/5] and [3/5] of this series would have to be
upstreamed in case you're still interested in pursuing them.
Note that a related DT property was introduced with 103dcf2ea2df.
Also note that you got some review comments on this series that
may still need to be addressed.
Just thought I'd let you know as I rediscovered this thread today
when flushing out old e-mails from my inbox.
Thanks,
Lukas