Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332232 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-11 17:50 +0100
    Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-02-11 20:10 +0100
      Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-11 21:30 +0100
        Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-02-12 09:00 +0100
          Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-12 10:40 +0100
            Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-17 18:00 +0100
              Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-18 08:20 +0100
                Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-18 10:50 +0100
                  Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Peter Hurley <peter@hurleysoftware.com> - 2016-02-18 18:00 +0100
                    Re: [PATCH v8 3/3] tty: 8250_omap: Use software emulated RS485  direction control Ильяс Гасанов   <torso.nafi@gmail.com> - 2016-02-18 19:20 +0100

#1332232 — 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.

[toc] | [next] | [standalone]

#1332321

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

[toc] | [prev] | [next] | [standalone]

#1332350

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.

[toc] | [prev] | [next] | [standalone]

#1332558

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

[toc] | [prev] | [next] | [standalone]

#1332613

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.

[toc] | [prev] | [next] | [standalone]

#1336579

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.

[toc] | [prev] | [next] | [standalone]

#1337097

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.

[toc] | [prev] | [next] | [standalone]

#1337202

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.

[toc] | [prev] | [next] | [standalone]

#1337533

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?

[toc] | [prev] | [next] | [standalone]

#1337610

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web