[PATCH v3 0/5] serial: 8250: Add rs485 emulation to 8250_dw

[PATCH v3 0/5] serial: 8250: Add rs485 emulation to 8250_dw

[Heiko Stuebner]

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/cover.1588505407.git.lukas@wunner.de

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

[PATCH v3 1/5] serial: 8520_port: Fix function param documentation

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

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 <andy.shevchenko@gmail.com>
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 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

Re: [PATCH v3 1/5] serial: 8520_port: Fix function param documentation

[Andy Shevchenko]

On Sun, May 17, 2020 at 11:56:06PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> 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 <andriy.shevchenko@linux.intel.com>

> Fixes: 058bc104f7ca ("serial: 8250: Generalize rs485 software emulation")
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  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

[PATCH v3 2/5] dt-bindings: serial: Add binding for rs485 receiver enable GPIO

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

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 <heiko.stuebner@theobroma-systems.com>
---
 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

[PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

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 <heiko.stuebner@theobroma-systems.com>
---
 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[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.


> @@ -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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Heiko Stübner]

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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Lukas Wunner]

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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Andy Shevchenko]

On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Lukas Wunner]

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 <heiko.stuebner@theobroma-systems.com>
> > 
> > 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Lukas Wunner]

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. %-/

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Maarten Brock]

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 <heiko.stuebner@theobroma-systems.com>
>> >
>> > 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Andy Shevchenko]

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 <heiko.stuebner@theobroma-systems.com>
> > > >
> > > > 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Maarten Brock]

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 <heiko.stuebner@theobroma-systems.com>
>> > > >
>> > > > 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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Andy Shevchenko]

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

Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Farouk Bouabid]

Hi,

On 17.05.20 23:56, Heiko Stuebner wrote:
> From: Heiko Stuebner<heiko.stuebner@theobroma-systems.com>
>
> 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<heiko.stuebner@theobroma-systems.com>
> ---
>   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 */
>   };

[PATCH v3 4/5] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

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 <giulio.benetti@micronovasrl.com>
[moved to use added UART_CAP_TEMT, use readx_poll_timeout]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 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

Re: [PATCH v3 4/5] serial: 8250: Handle implementations not having TEMT interrupt using em485

[Andy Shevchenko]

On Sun, May 17, 2020 at 11:56:09PM +0200, Heiko Stuebner wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> 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 <giulio.benetti@micronovasrl.com>

> [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

[PATCH v3 5/5] serial: 8250_dw: add em485 support

[Heiko Stuebner]

From: Giulio Benetti <giulio.benetti@micronovasrl.com>

Need to use rs485 transceiver so let's use existing em485 485 emulation
layer on top of 8250.

Add rs485_config callback to port and uses the standard em485 start and
stop helpers.

Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
[moved to use newly added start/stop helpers]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 51a7d3b19b39..36d364268a45 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -415,6 +415,9 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->rs485_config = serial8250_em485_config;
+	up->rs485_start_tx = serial8250_em485_start_tx;
+	up->rs485_stop_tx = serial8250_em485_stop_tx;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
2.25.1

Re: [PATCH v3 5/5] serial: 8250_dw: add em485 support

[Andy Shevchenko]

On Sun, May 17, 2020 at 11:56:10PM +0200, Heiko Stuebner wrote:
> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> 
> Need to use rs485 transceiver so let's use existing em485 485 emulation
> layer on top of 8250.
> 
> Add rs485_config callback to port and uses the standard em485 start and
> stop helpers.

Would it prevent to use native RS485 support?

...

>  	p->serial_out	= dw8250_serial_out;
>  	p->set_ldisc	= dw8250_set_ldisc;
>  	p->set_termios	= dw8250_set_termios;
> +	p->rs485_config = serial8250_em485_config;
> +	up->rs485_start_tx = serial8250_em485_start_tx;
> +	up->rs485_stop_tx = serial8250_em485_stop_tx;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/5] serial: 8250_dw: add em485 support

[Giulio Benetti]

Hi Andy,

Il 18/05/2020 17:21, Andy Shevchenko ha scritto:
> On Sun, May 17, 2020 at 11:56:10PM +0200, Heiko Stuebner wrote:
>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>
>> Need to use rs485 transceiver so let's use existing em485 485 emulation
>> layer on top of 8250.
>>
>> Add rs485_config callback to port and uses the standard em485 start and
>> stop helpers.
> 
> Would it prevent to use native RS485 support?

8250_dw doesn't have a native RS485 support, do you mean using hardware
RTS assertion? Anyway at the moment it's not present. This would be the
first rs485 support added to 8250_dw.

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

> ...
> 
>>   	p->serial_out	= dw8250_serial_out;
>>   	p->set_ldisc	= dw8250_set_ldisc;
>>   	p->set_termios	= dw8250_set_termios;
>> +	p->rs485_config = serial8250_em485_config;
>> +	up->rs485_start_tx = serial8250_em485_start_tx;
>> +	up->rs485_stop_tx = serial8250_em485_stop_tx;
> 

Re: [PATCH v3 5/5] serial: 8250_dw: add em485 support

[Andy Shevchenko]

On Tue, Feb 02, 2021 at 01:31:17AM +0100, Giulio Benetti wrote:
> Il 18/05/2020 17:21, Andy Shevchenko ha scritto:
> > On Sun, May 17, 2020 at 11:56:10PM +0200, Heiko Stuebner wrote:
> > > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > 
> > > Need to use rs485 transceiver so let's use existing em485 485 emulation
> > > layer on top of 8250.
> > > 
> > > Add rs485_config callback to port and uses the standard em485 start and
> > > stop helpers.
> > 
> > Would it prevent to use native RS485 support?
> 
> 8250_dw doesn't have a native RS485 support, do you mean using hardware
> RTS assertion? Anyway at the moment it's not present. This would be the
> first rs485 support added to 8250_dw.

DW v4.0+ has it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/5] serial: 8250_dw: add em485 support

[Giulio Benetti]

Il 02/02/2021 12:22, Andy Shevchenko ha scritto:
> On Tue, Feb 02, 2021 at 01:31:17AM +0100, Giulio Benetti wrote:
>> Il 18/05/2020 17:21, Andy Shevchenko ha scritto:
>>> On Sun, May 17, 2020 at 11:56:10PM +0200, Heiko Stuebner wrote:
>>>> From: Giulio Benetti <giulio.benetti@micronovasrl.com>
>>>>
>>>> Need to use rs485 transceiver so let's use existing em485 485 emulation
>>>> layer on top of 8250.
>>>>
>>>> Add rs485_config callback to port and uses the standard em485 start and
>>>> stop helpers.
>>>
>>> Would it prevent to use native RS485 support?
>>
>> 8250_dw doesn't have a native RS485 support, do you mean using hardware
>> RTS assertion? Anyway at the moment it's not present. This would be the
>> first rs485 support added to 8250_dw.
> 
> DW v4.0+ has it.

I have access to datasheet of DW v3, and I don't have access to hardware
with DW v4.0v Uart.
But I could add rs485emu for only UART version < 4.0 and I can find Uart
version by reading UCV register, would it be acceptable?

Best regards
-- 
Giulio Benetti
CTO

MICRONOVA SRL
Sede: Via A. Niedda 3 - 35010 Vigonza (PD)
Tel. 049/8931563 - Fax 049/8931346
Cod.Fiscale - P.IVA 02663420285
Capitale Sociale € 26.000 i.v.
Iscritta al Reg. Imprese di Padova N. 02663420285
Numero R.E.A. 258642

Re: [PATCH v3 5/5] serial: 8250_dw: add em485 support

[Andy Shevchenko]

On Fri, Feb 05, 2021 at 06:46:56PM +0100, Giulio Benetti wrote:
> Il 02/02/2021 12:22, Andy Shevchenko ha scritto:
> > On Tue, Feb 02, 2021 at 01:31:17AM +0100, Giulio Benetti wrote:
> > > Il 18/05/2020 17:21, Andy Shevchenko ha scritto:
> > > > On Sun, May 17, 2020 at 11:56:10PM +0200, Heiko Stuebner wrote:
> > > > > From: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > > > > 
> > > > > Need to use rs485 transceiver so let's use existing em485 485 emulation
> > > > > layer on top of 8250.
> > > > > 
> > > > > Add rs485_config callback to port and uses the standard em485 start and
> > > > > stop helpers.
> > > > 
> > > > Would it prevent to use native RS485 support?
> > > 
> > > 8250_dw doesn't have a native RS485 support, do you mean using hardware
> > > RTS assertion? Anyway at the moment it's not present. This would be the
> > > first rs485 support added to 8250_dw.
> > 
> > DW v4.0+ has it.
> 
> I have access to datasheet of DW v3, and I don't have access to hardware
> with DW v4.0v Uart.
> But I could add rs485emu for only UART version < 4.0 and I can find Uart
> version by reading UCV register, would it be acceptable?

I don't think we need to disable the possibility to have emulation even on v4+
IP since PCB can be designed that way. My point here, that whatever code you
add it should not prevent to use HW supported RS485, although I haven't heard
about support of it in the upstream (yet?).

And for the record, Synopsys hadn't added any bit into CPR to recognize that
feature, so only version check can be done in this case...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/5] serial: 8250: Add rs485 emulation to 8250_dw

[Lukas Wunner]

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