Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

Hello,

2016-02-01 21:09 GMT+03:00 "Matwey V. Kornilov" <matwey@sai.msu.ru>:
> +static int omap_8250_rs485_config(struct uart_port *port,
> +                                 struct serial_rs485 *rs485)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       /* Clamp the delays to [0, 100ms] */
> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
> +
> +       port->rs485 = *rs485;
> +
> +       /*
> +        * Both serial8250_em485_init and serial8250_em485_destroy
> +        * are idempotent
> +        */
> +       if (rs485->flags & SER_RS485_ENABLED) {
> +               int ret = serial8250_em485_init(up);
> +
> +               if (ret) {
> +                       rs485->flags &= ~SER_RS485_ENABLED;
> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
> +               }
> +               return ret;
> +       }
> +
> +       serial8250_em485_destroy(up);
> +
> +       return 0;
> +}

I think that an invocation of serial8250_em485_destroy() should be
performed inside omap8250_remove() as well. However there is a catch -
serial8250_get_port() is only reserved for suspend-resume power
management operations, and I am aware of no other means how to supply
an uart_8250_port to serial8250_em485_destroy() at that point.

Similarly, I got no idea on how to implement handling
"linux,rs485-enabled-at-boot-time" property without using
serial8250_get_port() directly (well, honestly I could try invoking
the corresponding ioctl somehow, but that's gonna be plain ugly at
least).

Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Matwey V. Kornilov]

2016-02-11 19:40 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> Hello,
>
> 2016-02-01 21:09 GMT+03:00 "Matwey V. Kornilov" <matwey@sai.msu.ru>:
>> +static int omap_8250_rs485_config(struct uart_port *port,
>> +                                 struct serial_rs485 *rs485)
>> +{
>> +       struct uart_8250_port *up = up_to_u8250p(port);
>> +
>> +       /* Clamp the delays to [0, 100ms] */
>> +       rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
>> +       rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
>> +
>> +       port->rs485 = *rs485;
>> +
>> +       /*
>> +        * Both serial8250_em485_init and serial8250_em485_destroy
>> +        * are idempotent
>> +        */
>> +       if (rs485->flags & SER_RS485_ENABLED) {
>> +               int ret = serial8250_em485_init(up);
>> +
>> +               if (ret) {
>> +                       rs485->flags &= ~SER_RS485_ENABLED;
>> +                       port->rs485.flags &= ~SER_RS485_ENABLED;
>> +               }
>> +               return ret;
>> +       }
>> +
>> +       serial8250_em485_destroy(up);
>> +
>> +       return 0;
>> +}
>
> I think that an invocation of serial8250_em485_destroy() should be
> performed inside omap8250_remove() as well. However there is a catch -
> serial8250_get_port() is only reserved for suspend-resume power
> management operations, and I am aware of no other means how to supply
> an uart_8250_port to serial8250_em485_destroy() at that point.

Thanks for pointing out. serial8250_unregister_port should set
serial8250_ports[line].em485 to NULL in order to prevent unneeded
activation when this struct is reused.

>
> Similarly, I got no idea on how to implement handling
> "linux,rs485-enabled-at-boot-time" property without using
> serial8250_get_port() directly (well, honestly I could try invoking
> the corresponding ioctl somehow, but that's gonna be plain ugly at
> least).

You would be able to use serial8250_em485_register in omap8250_probe
on &up before serial8250_register_8250_port(&up) if
serial8250_register_8250_port could replicate em485 member state.
But there are alternatives in implementation.
serial8250_register_8250_port may either copy pointer up->em485 to
uart->em485 or perform deep copy.

>
> Regards,
> Ilyas G.
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

2016-02-11 22:08 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> Thanks for pointing out. serial8250_unregister_port should set
> serial8250_ports[line].em485 to NULL in order to prevent unneeded
> activation when this struct is reused.

Then the allocated/initialized resources should be freed/released as well.

> You would be able to use serial8250_em485_register in omap8250_probe
> on &up before serial8250_register_8250_port(&up) if
> serial8250_register_8250_port could replicate em485 member state.
> But there are alternatives in implementation.

I'm considering adding the relevant code to the omap8250_startup and
omap8250_shutdown callback functions. However the serial driver
documentation claims that these don't have port->lock taken when
invoked, only port_sem.

> serial8250_register_8250_port may either copy pointer up->em485 to
> uart->em485 or perform deep copy.

Actually, "up" here in omap8250_probe is not a pointer but a struct
variable on stack, so copying the pointer to it is out of question.


Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Matwey V. Kornilov]

2016-02-11 23:19 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> 2016-02-11 22:08 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>> Thanks for pointing out. serial8250_unregister_port should set
>> serial8250_ports[line].em485 to NULL in order to prevent unneeded
>> activation when this struct is reused.
>
> Then the allocated/initialized resources should be freed/released as well.

Sure, I've already sketched the patch, but to test it, I need some
time to build 8250_omap as a module and setup networking console and
kgdb over network.

>
>> You would be able to use serial8250_em485_register in omap8250_probe
>> on &up before serial8250_register_8250_port(&up) if
>> serial8250_register_8250_port could replicate em485 member state.
>> But there are alternatives in implementation.
>
> I'm considering adding the relevant code to the omap8250_startup and
> omap8250_shutdown callback functions. However the serial driver
> documentation claims that these don't have port->lock taken when
> invoked, only port_sem.
>
>> serial8250_register_8250_port may either copy pointer up->em485 to
>> uart->em485 or perform deep copy.
>
> Actually, "up" here in omap8250_probe is not a pointer but a struct
> variable on stack, so copying the pointer to it is out of question.
>

up->em485 is always pointer, however you are right, that
serial8250_em485_init stores pointer to up when the timers are inited.
More complex issue here that serial8250_em485_init need to set RTS to
proper state and probably can't do that before
serial8250_register_8250_port. So omap8250_probe should directly or
indirectly use serial8250_get_port(priv->line) after
serial8250_register_8250_port. As for me, possible solution could be
to add a wrapper:

int serial8250_em485_init_by_line(int line) {
    struct uart_8250_port *uart = &serial8250_ports[line];
    int ret;
    unsigned long flags;

    spin_lock_irqsave(&uart->port.lock, flags);
    ret = serial8250_em485_init(uart).
    spin_unlock_irqrestore(&uart->port.lock, flags);

    return ret;
}

>
> Regards,
> Ilyas G.
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

2016-02-12 10:57 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> up->em485 is always pointer, however you are right, that
> serial8250_em485_init stores pointer to up when the timers are inited.
> More complex issue here that serial8250_em485_init need to set RTS to
> proper state and probably can't do that before
> serial8250_register_8250_port. So omap8250_probe should directly or
> indirectly use serial8250_get_port(priv->line) after
> serial8250_register_8250_port. As for me, possible solution could be
> to add a wrapper:
>
> int serial8250_em485_init_by_line(int line) {
>     struct uart_8250_port *uart = &serial8250_ports[line];
>     int ret;
>     unsigned long flags;
>
>     spin_lock_irqsave(&uart->port.lock, flags);
>     ret = serial8250_em485_init(uart).
>     spin_unlock_irqrestore(&uart->port.lock, flags);
>
>     return ret;
> }

Also, I noticed that in 8250_core.c the em485 tx handlers change the
MCR register itself. But in the case when RTS signal is itself
emulated, say by flipping a GPIO, a more generic approach would be
necessary, such as get_mctrl/set_mctrl, so that the use of helpers in
serial_mctrl_gpio.c is straightforward.

For example, in our design the RS232<->RS485 direction switch is
driven by a pin which cannot be pinmuxed as UART RTS but can be a
GPIO. Since it is already in production, making incompatible changes
to the design (and getting away with it) is not inherently feasible.


Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

Hello,

After testing the patch v8, I found two more glitches, which happen at
least in 4.1 kernel (I applied the changes to 8250_core.c, since
8250_port.c isn't made separate in 4.1):

1) When serial8250_em485_init is called from omap_8250_rs485_config,
there is a lockdep warning, and according to the stack trace it
happens at kmalloc invocation. Passing GFP_ATOMIC instead of
GFP_KERNEL in flags apparently fixes the problem.

2) Once a transmission happens and completes, the behavior of AM335x
UART becomes strange - it seemingly doesn't receive anything (or fails
to return to userspace the received payload). At the time of next
transmission (sometimes just before, sometimes just after, but never
during it) the single last received byte is returned via read, the
rest of the payload discarded. However when SER_RS485_RX_DURING_TX is
set, it works just okay. The problem is, this happens when clearly the
payload is not received during transmission, so it isn't liable to be
discarded upon clearing FIFO. Best guess the Rx interrupt which is
disabled in start_tx_rs485 upon calling serial8250_stop_rx isn't
properly restored.


Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

Also, forgot to mention that if serial8250_em485_init is called not
upon uart startup but elsewhere (upon port register for example), and
em485 is set, serial8250_do_startup should call
serial8250_em485_rts_after_send, or else RTS might be in wrong state
whenever the port device is opened, making it impossible to receive
data through RTS-controlled RS232<->RS485 hardware converters.


Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

2016-02-18 10:18 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
> Also, forgot to mention that if serial8250_em485_init is called not
> upon uart startup but elsewhere (upon port register for example), and
> em485 is set, serial8250_do_startup should call
> serial8250_em485_rts_after_send, or else RTS might be in wrong state
> whenever the port device is opened, making it impossible to receive
> data through RTS-controlled RS232<->RS485 hardware converters.

Just found out that it doesn't help. Actually, it is the behavior of
tty_open function to call tty_port_block_til_ready after startup, and
of the latter - to set RTS via ioctl if baud is not zero - both of
which cannot be overriden within the scope of the 8250 framework. The
reason this works in legacy omap_serial is that my GPIO which is used
to emulate RTS is not tied to mctrl there, so the call of
tty_port_block_til_ready does not affect its state.

Therefore, a check for SER_RS485_ENABLED needs to be introduced to
tty_port_block_til_ready.


Regards,
Ilyas G.

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Peter Hurley]

On 02/18/2016 01:46 AM, Ильяс Гасанов wrote:
> 2016-02-18 10:18 GMT+03:00 Ильяс Гасанов <torso.nafi@gmail.com>:
>> Also, forgot to mention that if serial8250_em485_init is called not
>> upon uart startup but elsewhere (upon port register for example), and
>> em485 is set, serial8250_do_startup should call
>> serial8250_em485_rts_after_send, or else RTS might be in wrong state
>> whenever the port device is opened, making it impossible to receive
>> data through RTS-controlled RS232<->RS485 hardware converters.
> 
> Just found out that it doesn't help. Actually, it is the behavior of
> tty_open function to call tty_port_block_til_ready after startup, and
> of the latter - to set RTS via ioctl if baud is not zero - both of
> which cannot be overriden within the scope of the 8250 framework. The
> reason this works in legacy omap_serial is that my GPIO which is used
> to emulate RTS is not tied to mctrl there, so the call of
> tty_port_block_til_ready does not affect its state.
> 
> Therefore, a check for SER_RS485_ENABLED needs to be introduced to
> tty_port_block_til_ready.

No, that's not the way.

First, there's plenty of driver hooks to properly manage the RTS level
for rs485.
Second, how are you running GPIO rts with the omap8250 driver?

Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Ильяс Гасанов]

Hello,

2016-02-18 19:50 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> No, that's not the way.
>
> First, there's plenty of driver hooks to properly manage the RTS level
> for rs485.

For now I'm considering to add the check to uart_dtr_rts in
serial_core.c. However, given the access to struct
tty_port_operations, I think it's also possible to use a
driver-specific hook if it makes more sense. The question is - where
exactly does it belong?

> Second, how are you running GPIO rts with the omap8250 driver?

Through hooking set_mctrl and get_mctrl and using the suggested
helpers from serial_mctrl_gpio.c. Though I had to introduce
uart_port.get_mctrl to serial_core.h (with set_mctrl already being
there) and serial8250_do_get_mctrl to serial_8250.h (same here,
serial8250_do_set_mctrl is already there).


Regards,
Ilyas G.

[PATCH v8 0/3] tty: Introduce software RS485 direction control support

[Matwey V. Kornilov]

Changes from v7:
 - rework comments to follow guidelines
 - minor style changes
Changes from v6:
 - minor style changes
 - timers are not IRQSAFE now
Changes from v5:
 - rs485_emul variable has been renamed to em485 to follow function names convention
Changes from v4:
 - Add commit message to 1/3
Changes from v3:
 - Completely redesigned.
Changes from v2:
 - Introduced SER_RS485_SOFTWARE to show that software implementation is being used
 - serial8250_rs485_config is located as required
 - Timers are used to implement delay_before and delay_after timeouts

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>

[PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485 direction control

[Matwey V. Kornilov]

Use software emulated RS485 direction control to provide RS485 API
existed in omap_serial driver. Note that 8250_omap issues interrupt
on shift register empty which is single prerequesite for using software
emulated RS485.

Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/tty/serial/8250/8250_omap.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index a2c0734..d710985 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -697,6 +697,36 @@ static void omap_8250_throttle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+static int omap_8250_rs485_config(struct uart_port *port,
+				  struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	/* Clamp the delays to [0, 100ms] */
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send, 100U);
+
+	port->rs485 = *rs485;
+
+	/*
+	 * Both serial8250_em485_init and serial8250_em485_destroy
+	 * are idempotent
+	 */
+	if (rs485->flags & SER_RS485_ENABLED) {
+		int ret = serial8250_em485_init(up);
+
+		if (ret) {
+			rs485->flags &= ~SER_RS485_ENABLED;
+			port->rs485.flags &= ~SER_RS485_ENABLED;
+		}
+		return ret;
+	}
+
+	serial8250_em485_destroy(up);
+
+	return 0;
+}
+
 static void omap_8250_unthrottle(struct uart_port *port)
 {
 	unsigned long flags;
@@ -1146,6 +1176,7 @@ static int omap8250_probe(struct platform_device *pdev)
 	up.port.shutdown = omap_8250_shutdown;
 	up.port.throttle = omap_8250_throttle;
 	up.port.unthrottle = omap_8250_unthrottle;
+	up.port.rs485_config = omap_8250_rs485_config;
 
 	if (pdev->dev.of_node) {
 		const struct of_device_id *id;
-- 
2.7.0