From: Lino Sanfilippo <[email protected]>
The following series includes cleanup and fixes around RS485 in the serial
core and uart drivers:
Patch 1: ar933x: Fix check for RS485 support
Patch 2: Remove superfluous code in ar933x.
Patch 3: Set the rs485 termination GPIO in the serial core. This is needed
since if the gpio is only accessible in sleepable context. It also
is a further step to make the RS485 handling more generic.
Patch 4: Move sanitizing of RS485 delays into an own function. This is in
preparation of patch 4.
Patch 5: Sanitize RS485 delays read from device tree.
Patch 6: Correct RS485 delays in binding documentation.
Patch 7: Remove redundant code in 8250_dwlib.
Patch 8: Remove redundant code in 8250-lpc18xx.
Changes in v4:
- fixed logical error found by
- capitalize "uart" and "gpio" in commit messages
Changes in v3:
- remove obsolete patch (due to changes by Ilpo)
- corrected and rephrase commit messages (pointed out by Andy)
- remove superfluous check (pointed out by Andy)
- separate ar933x UART bugfix and cleanup into different patches
(as suggested by Ilpo)
- put the ar933x fix at the beginning of the series (as suggested by Andy)
Changes in v2:
- print a warning if termination GPIO is specified in DT/ACPI but is not
supported by driver
- fixed commit message for devtree documentation (as suggested by Andy)
- fixed code comment
- added patch 7
Lino Sanfilippo (8):
serial: ar933x: Fix check for RS485 support
serial: ar933x: Remove superfluous code in ar933x_config_rs485()
serial: core, 8250: set RS485 termination GPIO in serial core
serial: core: move sanitizing of RS485 delays into own function
serial: core: sanitize RS485 delays read from device tree
dt_bindings: rs485: Correct delay values
serial: 8250_dwlib: remove redundant sanity check for RS485 flags
serial: 8250: lpc18xx: Remove redundant sanity check for RS485 flags
.../devicetree/bindings/serial/rs485.yaml | 4 +-
drivers/tty/serial/8250/8250_dwlib.c | 10 +---
drivers/tty/serial/8250/8250_lpc18xx.c | 6 +-
drivers/tty/serial/8250/8250_port.c | 3 -
drivers/tty/serial/ar933x_uart.c | 18 ++----
drivers/tty/serial/serial_core.c | 60 ++++++++++++-------
6 files changed, 50 insertions(+), 51 deletions(-)
base-commit: 7e5b4322cde067e1d0f1bf8f490e93f664a7c843
--
2.25.1
From: Lino Sanfilippo <[email protected]>
In serial8250_em485_config() the termination GPIO is set with the uart_port
spinlock held. This is an issue if setting the GPIO line can sleep (e.g.
since the concerning GPIO expander is connected via SPI or I2C).
Fix this by setting the termination line outside of the uart_port spinlock
in the serial core and using gpiod_set_value_cansleep() which instead of
gpiod_set_value() allows it to sleep.
Beside fixing the termination GPIO line setting for the 8250 driver this
change also makes setting the termination GPIO generic for all UART
drivers.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/8250/8250_port.c | 3 ---
drivers/tty/serial/serial_core.c | 12 ++++++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ed2a606f2da7..72252d956f17 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
}
- gpiod_set_value(port->rs485_term_gpio,
- rs485->flags & SER_RS485_TERMINATE_BUS);
-
/*
* Both serial8250_em485_init() and serial8250_em485_destroy()
* are idempotent.
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1db44cde76f6..047ec51dbd41 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1358,12 +1358,23 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
memset(rs485->padding1, 0, sizeof(rs485->padding1));
}
+static void uart_set_rs485_termination(struct uart_port *port,
+ const struct serial_rs485 *rs485)
+{
+ if (!(rs485->flags & SER_RS485_ENABLED))
+ return;
+
+ gpiod_set_value_cansleep(port->rs485_term_gpio,
+ !!(rs485->flags & SER_RS485_TERMINATE_BUS));
+}
+
int uart_rs485_config(struct uart_port *port)
{
struct serial_rs485 *rs485 = &port->rs485;
int ret;
uart_sanitize_serial_rs485(port, rs485);
+ uart_set_rs485_termination(port, rs485);
ret = port->rs485_config(port, NULL, rs485);
if (ret)
@@ -1406,6 +1417,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
if (ret)
return ret;
uart_sanitize_serial_rs485(port, &rs485);
+ uart_set_rs485_termination(port, &rs485);
spin_lock_irqsave(&port->lock, flags);
ret = port->rs485_config(port, &tty->termios, &rs485);
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Currently the documentation claims that a maximum of 1000 msecs is allowed
for RTS delays. However nothing actually checks the values read from device
tree/ACPI and so it is possible to set much higher values.
There is already a maximum of 100 ms enforced for RTS delays that are set
via the UART TIOCSRS485 ioctl. To be consistent with that use the same
limit for DT/ACPI values.
Although this change is visible to userspace the risk of breaking anything
when reducing the max delays from 1000 to 100 ms should be very low, since
100 ms is already a very high maximum for delays that are usually rather in
the usecs range.
Signed-off-by: Lino Sanfilippo <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index f2c9c9fe6aa7..90a1bab40f05 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -22,12 +22,12 @@ properties:
- description: Delay between rts signal and beginning of data sent in
milliseconds. It corresponds to the delay before sending data.
default: 0
- maximum: 1000
+ maximum: 100
- description: Delay between end of data sent and rts signal in milliseconds.
It corresponds to the delay after sending data and actual release
of the line.
default: 0
- maximum: 1000
+ maximum: 100
rs485-rts-active-low:
description: drive RTS low when sending (default is high).
--
2.25.1
From: Lino Sanfilippo <[email protected]>
RS485 is not possible without an RTS GPIO regardless of whether RS485 is
enabled at boot time or not. So correct the concerning check in the probe()
function.
Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/ar933x_uart.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index f931ecbc0bc0..f7b4638d69e5 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -798,11 +798,12 @@ static int ar933x_uart_probe(struct platform_device *pdev)
up->rts_gpiod = mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS);
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !up->rts_gpiod) {
- dev_err(&pdev->dev, "lacking rts-gpio, disabling RS485\n");
- port->rs485.flags &= ~SER_RS485_ENABLED;
+ if (!up->rts_gpiod) {
port->rs485_supported = ar933x_no_rs485;
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ dev_err(&pdev->dev, "lacking rts-gpio, disabling RS485\n");
+ port->rs485.flags &= ~SER_RS485_ENABLED;
+ }
}
#ifdef CONFIG_SERIAL_AR933X_CONSOLE
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Move the sanitizing of RS485 delays out of uart_sanitize_serial_rs485()
into the new function uart_sanitize_serial_rs485_delays().
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/serial_core.c | 46 ++++++++++++++++++--------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 047ec51dbd41..3158f05a328c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1302,27 +1302,9 @@ static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *r
return 0;
}
-static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+static void uart_sanitize_serial_rs485_delays(struct uart_port *port,
+ struct serial_rs485 *rs485)
{
- u32 supported_flags = port->rs485_supported.flags;
-
- if (!(rs485->flags & SER_RS485_ENABLED)) {
- memset(rs485, 0, sizeof(*rs485));
- return;
- }
-
- /* pick sane settings if the user hasn't */
- if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
- !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
- !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
- dev_warn_ratelimited(port->dev,
- "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
- port->name, port->line);
- rs485->flags |= SER_RS485_RTS_ON_SEND;
- rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
- supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
- }
-
if (!port->rs485_supported.delay_rts_before_send) {
if (rs485->delay_rts_before_send) {
dev_warn_ratelimited(port->dev,
@@ -1350,9 +1332,33 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
"%s (%d): RTS delay after sending clamped to %u ms\n",
port->name, port->line, rs485->delay_rts_after_send);
}
+}
+
+static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+{
+ u32 supported_flags = port->rs485_supported.flags;
+
+ if (!(rs485->flags & SER_RS485_ENABLED)) {
+ memset(rs485, 0, sizeof(*rs485));
+ return;
+ }
+
+ /* Pick sane settings if the user hasn't */
+ if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
+ !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+ !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+ dev_warn_ratelimited(port->dev,
+ "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
+ port->name, port->line);
+ rs485->flags |= SER_RS485_RTS_ON_SEND;
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+ supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
+ }
rs485->flags &= supported_flags;
+ uart_sanitize_serial_rs485_delays(port, rs485);
+
/* Return clean padding area to userspace */
memset(rs485->padding0, 0, sizeof(rs485->padding0));
memset(rs485->padding1, 0, sizeof(rs485->padding1));
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Currently the RTS delays set via device tree are not clamped to a maximum
value although the device tree bindings documentation for RS485 claims that
only a maximum of 1000 msecs is allowed.
So clamp the values to avoid arbitrary high delay settings. However clamp
the values to 100 instead of 1000 msecs to be consistent which the maximum
that is allowed when setting the delays from userspace via the UART ioctl
TIOCSRS485.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/serial_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3158f05a328c..ac198d0d4c80 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3395,6 +3395,8 @@ int uart_get_rs485_mode(struct uart_port *port)
rs485conf->delay_rts_after_send = 0;
}
+ uart_sanitize_serial_rs485_delays(port, rs485conf);
+
/*
* Clear full-duplex and enabled flags, set RTS polarity to active high
* to get to a defined state with the following properties:
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Before the drivers rs485_config() function is called the serial core
already ensures that only one of both options RTS on send or RTS after send
is set. So remove the concerning sanity check in the driver function to
avoid redundancy.
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_dwlib.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 2c3b1468bd88..dbe4d44f60d4 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -187,16 +187,10 @@ static int dw8250_rs485_config(struct uart_port *p, struct ktermios *termios,
if (rs485->flags & SER_RS485_ENABLED) {
tcr |= DW_UART_TCR_RS485_EN;
- if (rs485->flags & SER_RS485_RX_DURING_TX) {
+ if (rs485->flags & SER_RS485_RX_DURING_TX)
tcr |= DW_UART_TCR_XFER_MODE_DE_DURING_RE;
- } else {
- /* HW does not support same DE level for tx and rx */
- if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
- !(rs485->flags & SER_RS485_RTS_AFTER_SEND))
- return -EINVAL;
-
+ else
tcr |= DW_UART_TCR_XFER_MODE_DE_OR_RE;
- }
dw8250_writel_ext(p, DW_UART_DE_EN, 1);
dw8250_writel_ext(p, DW_UART_RE_EN, 1);
} else {
--
2.25.1
On Sun, Jul 10, 2022 at 06:44:34PM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> The following series includes cleanup and fixes around RS485 in the serial
> core and uart drivers:
>
> Patch 1: ar933x: Fix check for RS485 support
> Patch 2: Remove superfluous code in ar933x.
> Patch 3: Set the rs485 termination GPIO in the serial core. This is needed
> since if the gpio is only accessible in sleepable context. It also
> is a further step to make the RS485 handling more generic.
> Patch 4: Move sanitizing of RS485 delays into an own function. This is in
> preparation of patch 4.
> Patch 5: Sanitize RS485 delays read from device tree.
> Patch 6: Correct RS485 delays in binding documentation.
> Patch 7: Remove redundant code in 8250_dwlib.
> Patch 8: Remove redundant code in 8250-lpc18xx.
> Changes in v4:
> - fixed logical error found by
> - capitalize "uart" and "gpio" in commit messages
Please, avoid sending sequential version of the series more often than once per
a few days, recommended interval is one week.
--
With Best Regards,
Andy Shevchenko
Hi,
On 10.07.22 20:43, Andy Shevchenko wrote:
> On Sun, Jul 10, 2022 at 06:44:34PM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <[email protected]>
>>
>> The following series includes cleanup and fixes around RS485 in the serial
>> core and uart drivers:
>>
>> Patch 1: ar933x: Fix check for RS485 support
>> Patch 2: Remove superfluous code in ar933x.
>> Patch 3: Set the rs485 termination GPIO in the serial core. This is needed
>> since if the gpio is only accessible in sleepable context. It also
>> is a further step to make the RS485 handling more generic.
>> Patch 4: Move sanitizing of RS485 delays into an own function. This is in
>> preparation of patch 4.
>> Patch 5: Sanitize RS485 delays read from device tree.
>> Patch 6: Correct RS485 delays in binding documentation.
>> Patch 7: Remove redundant code in 8250_dwlib.
>> Patch 8: Remove redundant code in 8250-lpc18xx.
>
>> Changes in v4:
>> - fixed logical error found by
>> - capitalize "uart" and "gpio" in commit messages
>
> Please, avoid sending sequential version of the series more often than once per
> a few days, recommended interval is one week.
>
sorry, this was due to the error found by the kernel test robot. I was not sure
if to wait, resend with the same version number or send the next version.
I guess waiting a few days would have been the best option. Will do so next
time.
Regards,
Lino