From: Heiko Stuebner <[email protected]>
The RE signal is used to control the duplex mode of transmissions,
aka receiving data while sending in full duplex mode, while stopping
receiving data in half-duplex mode.
On a number of boards the !RE signal is tied to ground so reception
is always enabled except if the UART allows disabling the receiver.
This can be taken advantage of to implement half-duplex mode - like
done on 8250_bcm2835aux.
Another solution is to tie !RE to RTS always forcing half-duplex mode.
And finally there is the option to control the RE signal separately,
like done here by introducing a new rs485-specific gpio that can be
set depending on the RX_DURING_TX setting in the common em485 callbacks.
Signed-off-by: Heiko Stuebner <[email protected]>
---
drivers/tty/serial/8250/8250_port.c | 7 ++++++-
drivers/tty/serial/serial_core.c | 10 ++++++++++
include/linux/serial_core.h | 1 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6975bd3ecb7d..9e8fec85d1a3 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1444,6 +1444,7 @@ static void serial8250_stop_rx(struct uart_port *port)
void serial8250_em485_stop_tx(struct uart_8250_port *p)
{
unsigned char mcr = serial8250_in_MCR(p);
+ struct uart_port *port = &p->port;
if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
mcr |= UART_MCR_RTS;
@@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
* Enable previously disabled RX interrupts.
*/
if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+ gpiod_set_value(port->rs485_re_gpio, 1);
serial8250_clear_and_reinit_fifos(p);
p->ier |= UART_IER_RLSI | UART_IER_RDI;
@@ -1597,9 +1599,12 @@ static inline void __start_tx(struct uart_port *port)
void serial8250_em485_start_tx(struct uart_8250_port *up)
{
unsigned char mcr = serial8250_in_MCR(up);
+ struct uart_port *port = &up->port;
- if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+ gpiod_set_value(port->rs485_re_gpio, 0);
serial8250_stop_rx(&up->port);
+ }
if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
mcr |= UART_MCR_RTS;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 15ad8737b72b..c0d3ab32b49a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3149,6 +3149,16 @@ int uart_get_rs485_mode(struct uart_port *port)
return ret;
}
+ port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(port->rs485_re_gpio)) {
+ ret = PTR_ERR(port->rs485_re_gpio);
+ port->rs485_re_gpio = NULL;
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Cannot get rs485-rx-enable-gpios\n");
+ return ret;
+ }
+
return 0;
}
EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 108f95411471..dfe18ddb1674 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -253,6 +253,7 @@ struct uart_port {
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
struct serial_rs485 rs485;
struct gpio_desc *rs485_term_gpio; /* enable RS485 bus termination */
+ struct gpio_desc *rs485_re_gpio; /* gpio RS485 receive enable */
struct serial_iso7816 iso7816;
void *private_data; /* generic platform data pointer */
};
--
2.25.1
On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> @@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
> * Enable previously disabled RX interrupts.
> */
> if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + gpiod_set_value(port->rs485_re_gpio, 1);
> serial8250_clear_and_reinit_fifos(p);
>
> p->ier |= UART_IER_RLSI | UART_IER_RDI;
The added line needs to be conditional on if (port->rs485_re_gpio)
because the gpiod could be NULL and gpiod_set_value() doesn't check
for that.
> @@ -1597,9 +1599,12 @@ static inline void __start_tx(struct uart_port *port)
> void serial8250_em485_start_tx(struct uart_8250_port *up)
> {
> unsigned char mcr = serial8250_in_MCR(up);
> + struct uart_port *port = &up->port;
>
> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + gpiod_set_value(port->rs485_re_gpio, 0);
> serial8250_stop_rx(&up->port);
> + }
Same here.
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -253,6 +253,7 @@ struct uart_port {
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> struct serial_rs485 rs485;
> struct gpio_desc *rs485_term_gpio; /* enable RS485 bus termination */
> + struct gpio_desc *rs485_re_gpio; /* gpio RS485 receive enable */
Nit: I'd probably document this as "enable RS485 receiver" because it's
already apparent from the variable type and name that it's a gpio,
making it unnecessary to repeat that in the code comment. But I guess
that's a matter of personal preference.
There's something else: You need to amend serial8250_em485_config()
to toggle the GPIO depending on whether SER_RS485_RX_DURING_TX is
set. Right now you enable the receiver by default and then disable
it when starting to transmit if half-duplex mode is selected and
likewise re-enable it when stopping to transmit. But user space
may write some stuff to the tty while in half-duplex mode, then
immediately issue a TIOCSRS485 ioctl to switch to full-duplex mode.
If the ->rs485_config callback is executed while transmitting is
still ongoing, then you'll not re-enable the receiver when transmitting
finally stops. The ->rs485_config callback is invoked under the
uart port spinlock but the lock may be briefly released and later
re-acquired by the IRQ handler if the TX FIFO is full. (Unless
I'm missing something.)
Thanks,
Lukas
Am Montag, 18. Mai 2020, 06:50:06 CEST schrieb Lukas Wunner:
> On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> > @@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
> > * Enable previously disabled RX interrupts.
> > */
> > if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> > + gpiod_set_value(port->rs485_re_gpio, 1);
> > serial8250_clear_and_reinit_fifos(p);
> >
> > p->ier |= UART_IER_RLSI | UART_IER_RDI;
>
> The added line needs to be conditional on if (port->rs485_re_gpio)
> because the gpiod could be NULL and gpiod_set_value() doesn't check
> for that.
Need to look deeper at the other comment below, but gpiod_set_value does
check for NULL ;-)
void gpiod_set_value(struct gpio_desc *desc, int value)
{
VALIDATE_DESC_VOID(desc);
[...]
which expands to
#define VALIDATE_DESC_VOID(desc) do { \
int __valid = validate_desc(desc, __func__); \
if (__valid <= 0) \
return; \
} while (0)
which does
*/
static int validate_desc(const struct gpio_desc *desc, const char *func)
{
if (!desc)
return 0;
[...]
Heiko
On Mon, May 18, 2020 at 10:04:05AM +0200, Heiko St?bner wrote:
> Am Montag, 18. Mai 2020, 06:50:06 CEST schrieb Lukas Wunner:
> > On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> > > @@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
> > > * Enable previously disabled RX interrupts.
> > > */
> > > if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> > > + gpiod_set_value(port->rs485_re_gpio, 1);
> > > serial8250_clear_and_reinit_fifos(p);
> > >
> > > p->ier |= UART_IER_RLSI | UART_IER_RDI;
> >
> > The added line needs to be conditional on if (port->rs485_re_gpio)
> > because the gpiod could be NULL and gpiod_set_value() doesn't check
> > for that.
>
> Need to look deeper at the other comment below, but gpiod_set_value does
> check for NULL ;-)
I'll be damned. That means I should respin my patch [4/4] and
drop the NULL check. :-/
Thanks!
Lukas
On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <[email protected]>
>
> The RE signal is used to control the duplex mode of transmissions,
> aka receiving data while sending in full duplex mode, while stopping
> receiving data in half-duplex mode.
>
> On a number of boards the !RE signal is tied to ground so reception
> is always enabled except if the UART allows disabling the receiver.
> This can be taken advantage of to implement half-duplex mode - like
> done on 8250_bcm2835aux.
>
> Another solution is to tie !RE to RTS always forcing half-duplex mode.
>
> And finally there is the option to control the RE signal separately,
> like done here by introducing a new rs485-specific gpio that can be
> set depending on the RX_DURING_TX setting in the common em485 callbacks.
...
> + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
> + GPIOD_OUT_HIGH);
While reviewing some other patch I realized that people are missing the point
of these GPIO flags when pin is declared to be output.
HIGH here means "asserted" (consider active-high vs. active-low in general). Is
that the intention here?
Lukas, same question to your patch.
--
With Best Regards,
Andy Shevchenko
On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote:
> On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> > From: Heiko Stuebner <[email protected]>
> >
> > The RE signal is used to control the duplex mode of transmissions,
> > aka receiving data while sending in full duplex mode, while stopping
> > receiving data in half-duplex mode.
> >
> > On a number of boards the !RE signal is tied to ground so reception
> > is always enabled except if the UART allows disabling the receiver.
> > This can be taken advantage of to implement half-duplex mode - like
> > done on 8250_bcm2835aux.
> >
> > Another solution is to tie !RE to RTS always forcing half-duplex mode.
> >
> > And finally there is the option to control the RE signal separately,
> > like done here by introducing a new rs485-specific gpio that can be
> > set depending on the RX_DURING_TX setting in the common em485 callbacks.
>
> ...
>
> > + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
> > + GPIOD_OUT_HIGH);
>
> While reviewing some other patch I realized that people are missing the
> point of these GPIO flags when pin is declared to be output.
>
> HIGH here means "asserted" (consider active-high vs. active-low in
> general). Is that the intention here?
>
> Lukas, same question to your patch.
Yes. "High", i.e. asserted, means "termination enabled" in the case of
my patch and "receiver enabled" in the case of Heiko's patch.
For termination, the default is "low", i.e. termination disabled.
I talked to our hardware engineers and they said that disabling
termination by default is the safe choice: If multiple devices
on the RS485 bus enable termination, then no communication may
be possible at all. Whereas if termination is disabled,
communication should always work at least for short cables.
And for longer cables, users may have to disable it using the
TIOCSRS485 ioctl.
In the case of Heiko's patches, the default is "high", i.e.
the receiver is enabled by default so you're able to receive
data over the bus after opening the tty. Once you start
transmitting, the GPIO may be switched to low for the duration
of the transmission if half-duplex mode is enabled.
Thanks,
Lukas
On Mon, May 18, 2020 at 05:22:47PM +0200, Lukas Wunner wrote:
> And for longer cables, users may have to disable it using the
> TIOCSRS485 ioctl.
Sorry, I meant *enable* here. %-/
On 2020-05-18 17:22, Lukas Wunner wrote:
> On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote:
>> On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
>> > From: Heiko Stuebner <[email protected]>
>> >
>> > The RE signal is used to control the duplex mode of transmissions,
>> > aka receiving data while sending in full duplex mode, while stopping
>> > receiving data in half-duplex mode.
>> >
>> > On a number of boards the !RE signal is tied to ground so reception
>> > is always enabled except if the UART allows disabling the receiver.
>> > This can be taken advantage of to implement half-duplex mode - like
>> > done on 8250_bcm2835aux.
>> >
>> > Another solution is to tie !RE to RTS always forcing half-duplex mode.
>> >
>> > And finally there is the option to control the RE signal separately,
>> > like done here by introducing a new rs485-specific gpio that can be
>> > set depending on the RX_DURING_TX setting in the common em485 callbacks.
>>
>> ...
>>
>> > + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
>> > + GPIOD_OUT_HIGH);
>>
>> While reviewing some other patch I realized that people are missing
>> the
>> point of these GPIO flags when pin is declared to be output.
>>
>> HIGH here means "asserted" (consider active-high vs. active-low in
>> general). Is that the intention here?
>>
>> Lukas, same question to your patch.
>
> Yes. "High", i.e. asserted, means "termination enabled" in the case of
> my patch and "receiver enabled" in the case of Heiko's patch.
But "High" on a gpio would disable the receiver when connected to !RE.
Maarten
On Mon, May 18, 2020 at 06:13:16PM +0200, Maarten Brock wrote:
> On 2020-05-18 17:22, Lukas Wunner wrote:
> > On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote:
> > > On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <[email protected]>
> > > >
> > > > The RE signal is used to control the duplex mode of transmissions,
> > > > aka receiving data while sending in full duplex mode, while stopping
> > > > receiving data in half-duplex mode.
> > > >
> > > > On a number of boards the !RE signal is tied to ground so reception
> > > > is always enabled except if the UART allows disabling the receiver.
> > > > This can be taken advantage of to implement half-duplex mode - like
> > > > done on 8250_bcm2835aux.
> > > >
> > > > Another solution is to tie !RE to RTS always forcing half-duplex mode.
> > > >
> > > > And finally there is the option to control the RE signal separately,
> > > > like done here by introducing a new rs485-specific gpio that can be
> > > > set depending on the RX_DURING_TX setting in the common em485 callbacks.
> > >
> > > ...
> > >
> > > > + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
> > > > + GPIOD_OUT_HIGH);
> > >
> > > While reviewing some other patch I realized that people are missing
> > > the
> > > point of these GPIO flags when pin is declared to be output.
> > >
> > > HIGH here means "asserted" (consider active-high vs. active-low in
> > > general). Is that the intention here?
> > >
> > > Lukas, same question to your patch.
> >
> > Yes. "High", i.e. asserted, means "termination enabled" in the case of
> > my patch and "receiver enabled" in the case of Heiko's patch.
>
> But "High" on a gpio would disable the receiver when connected to !RE.
No, that's exactly the point of the terminology (asserted means active whatever
polarity it is). You need to define active-low in GPIO description.
--
With Best Regards,
Andy Shevchenko
On 2020-05-18 18:35, Andy Shevchenko wrote:
> On Mon, May 18, 2020 at 06:13:16PM +0200, Maarten Brock wrote:
>> On 2020-05-18 17:22, Lukas Wunner wrote:
>> > On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote:
>> > > On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
>> > > > From: Heiko Stuebner <[email protected]>
>> > > >
>> > > > The RE signal is used to control the duplex mode of transmissions,
>> > > > aka receiving data while sending in full duplex mode, while stopping
>> > > > receiving data in half-duplex mode.
>> > > >
>> > > > On a number of boards the !RE signal is tied to ground so reception
>> > > > is always enabled except if the UART allows disabling the receiver.
>> > > > This can be taken advantage of to implement half-duplex mode - like
>> > > > done on 8250_bcm2835aux.
>> > > >
>> > > > Another solution is to tie !RE to RTS always forcing half-duplex mode.
>> > > >
>> > > > And finally there is the option to control the RE signal separately,
>> > > > like done here by introducing a new rs485-specific gpio that can be
>> > > > set depending on the RX_DURING_TX setting in the common em485 callbacks.
>> > >
>> > > ...
>> > >
>> > > > + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
>> > > > + GPIOD_OUT_HIGH);
>> > >
>> > > While reviewing some other patch I realized that people are missing
>> > > the
>> > > point of these GPIO flags when pin is declared to be output.
>> > >
>> > > HIGH here means "asserted" (consider active-high vs. active-low in
>> > > general). Is that the intention here?
>> > >
>> > > Lukas, same question to your patch.
>> >
>> > Yes. "High", i.e. asserted, means "termination enabled" in the case of
>> > my patch and "receiver enabled" in the case of Heiko's patch.
>>
>> But "High" on a gpio would disable the receiver when connected to !RE.
>
> No, that's exactly the point of the terminology (asserted means active
> whatever
> polarity it is). You need to define active-low in GPIO description.
Is there anything wrong with defining GPIOD_OUT_ACTIVE or
GPIOD_OUT_ASSERTED
for this very purpose? May I suggest to deprecate GPIOD_OUT_HIGH and
replace it?
Maarten
On Mon, May 18, 2020 at 07:05:11PM +0200, Maarten Brock wrote:
> On 2020-05-18 18:35, Andy Shevchenko wrote:
> > On Mon, May 18, 2020 at 06:13:16PM +0200, Maarten Brock wrote:
> > > On 2020-05-18 17:22, Lukas Wunner wrote:
> > > > On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote:
> > > > > On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
...
> > > But "High" on a gpio would disable the receiver when connected to !RE.
> >
> > No, that's exactly the point of the terminology (asserted means active
> > whatever
> > polarity it is). You need to define active-low in GPIO description.
>
> Is there anything wrong with defining GPIOD_OUT_ACTIVE or GPIOD_OUT_ASSERTED
> for this very purpose? May I suggest to deprecate GPIOD_OUT_HIGH and replace
> it?
Please, ask GPIO maintainers, I'm not one.
--
With Best Regards,
Andy Shevchenko
Hi,
On 17.05.20 23:56, Heiko Stuebner wrote:
> From: Heiko Stuebner<[email protected]>
>
> The RE signal is used to control the duplex mode of transmissions,
> aka receiving data while sending in full duplex mode, while stopping
> receiving data in half-duplex mode.
>
> On a number of boards the !RE signal is tied to ground so reception
> is always enabled except if the UART allows disabling the receiver.
> This can be taken advantage of to implement half-duplex mode - like
> done on 8250_bcm2835aux.
>
> Another solution is to tie !RE to RTS always forcing half-duplex mode.
>
> And finally there is the option to control the RE signal separately,
> like done here by introducing a new rs485-specific gpio that can be
> set depending on the RX_DURING_TX setting in the common em485 callbacks.
>
> Signed-off-by: Heiko Stuebner<[email protected]>
> ---
> drivers/tty/serial/8250/8250_port.c | 7 ++++++-
> drivers/tty/serial/serial_core.c | 10 ++++++++++
> include/linux/serial_core.h | 1 +
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 6975bd3ecb7d..9e8fec85d1a3 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1444,6 +1444,7 @@ static void serial8250_stop_rx(struct uart_port *port)
> void serial8250_em485_stop_tx(struct uart_8250_port *p)
> {
> unsigned char mcr = serial8250_in_MCR(p);
> + struct uart_port *port = &p->port;
>
> if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
> mcr |= UART_MCR_RTS;
> @@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
> * Enable previously disabled RX interrupts.
> */
> if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + gpiod_set_value(port->rs485_re_gpio, 1);
> serial8250_clear_and_reinit_fifos(p);
>
> p->ier |= UART_IER_RLSI | UART_IER_RDI;
> @@ -1597,9 +1599,12 @@ static inline void __start_tx(struct uart_port *port)
> void serial8250_em485_start_tx(struct uart_8250_port *up)
> {
> unsigned char mcr = serial8250_in_MCR(up);
> + struct uart_port *port = &up->port;
>
> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> + gpiod_set_value(port->rs485_re_gpio, 0);
> serial8250_stop_rx(&up->port);
> + }
>
> if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> mcr |= UART_MCR_RTS;
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 15ad8737b72b..c0d3ab32b49a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3149,6 +3149,16 @@ int uart_get_rs485_mode(struct uart_port *port)
> return ret;
> }
>
> + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
> + GPIOD_OUT_HIGH);
The half duplex emulation can be disabled on-boot or later-on using
ioctls in systems that can switch between half and full duplex modes. In
that case this gpio should probably be released as it can probably
prevent enabling the transceiver-sender mode. I suggest we set the gpio
to output active in the em485_init callback and stop driving it in the
em485_destroy callback.
Regards
> + if (IS_ERR(port->rs485_re_gpio)) {
> + ret = PTR_ERR(port->rs485_re_gpio);
> + port->rs485_re_gpio = NULL;
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Cannot get rs485-rx-enable-gpios\n");
> + return ret;
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 108f95411471..dfe18ddb1674 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -253,6 +253,7 @@ struct uart_port {
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> struct serial_rs485 rs485;
> struct gpio_desc *rs485_term_gpio; /* enable RS485 bus termination */
> + struct gpio_desc *rs485_re_gpio; /* gpio RS485 receive enable */
> struct serial_iso7816 iso7816;
> void *private_data; /* generic platform data pointer */
> };