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

Groups > linux.kernel > #1282567 > unrolled thread

Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 Peter Hurley <peter@hurleysoftware.com> - 2015-12-03 00:30 +0100
    Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2015-12-03 07:00 +0100
      Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 Peter Hurley <peter@hurleysoftware.com> - 2015-12-03 15:50 +0100
        Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2015-12-03 18:40 +0100
          Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 Peter Hurley <peter@hurleysoftware.com> - 2015-12-03 20:50 +0100
            Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag  for struct serial_rs485 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2015-12-04 19:00 +0100

#1282567 — Re: [PATCH v3 2/5] tty: Introduce SER_RS485_SOFTWARE read-only flag for struct serial_rs485

On 11/18/2015 02:49 PM, Matwey V. Kornilov wrote:
> 2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>> 2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
[...]
>>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>>>> pointless impossible to correctly set flag for all eternity.
>>>>>>
>>>>>> Please explain the correct setting for this flag when a device driver
>>>>>> uses hardware or software or a mix according to what the silicon is
>>>>>> capable of and what values are requested ? How will an application use the
>>>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>>>> silicon bug and in a future 4.x release turns an implementation from
>>>>>> hardware to software - will they have to lie about the flag to avoid
>>>>>> breaking their application code - that strikes me as a bad thing.
>>>>>
>>>>> The existing driver behavior is already significantly variant and needs
>>>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>>>
>>>>> mcfuart         ignores delay values, delays unsupported
>>>>> imx             clamps delay values to 0, delays unsupported
>>>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>>>                 clamps delay_rts_after_send to max h/w value
>>>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>>>> * implements delay(s) in software
>>>>>
>>>>> The omap-serial emulation should not have been merged in its current form.
>>>>>
>>>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>>>> be clamped to 0. The application should check the RS485 settings returned by
>>>>> TIOCSRS485 to determine how the driver set them.
>>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>>>
>>>> But the similar could be true for minimal supported delay. If user
>>>> requires delay which is less than lower bound, the delay is raised to
>>>> the lower bound. If user requires delay which is greater than upper
>>>> bound, the delay is set to the upper bound. Then software
>>>> implementation could use (tx fifo size / baudrate) as lower bound for
>>>> delay_after_send.
>>>
>>> From the application point-of-view (really the only relevant semantics),
>>> delay_dts_after_send refers to the number of milliseconds to delay the
>>> toggle of RTS after the last bit has been _transmitted_.

Is there consensus then about what the semantics of unsupported RS485 delay
values are? I (or someone else) can trivially add the documentation and
fixes to the existing in-tree drivers.


>>> A couple of possibilities for improving the emulation are:
>>> 1) Optionally using an HR timer for sub-jiffy turnaround.
>>> 2) Only supporting 8250-based hardware that can be set to interrupt when
>>>    both tx fifo and transmitter shift register are empty.
>>
>> This is to support the RS485 API with already exists in omap_serrial,
>> but not in 8250_omap. And OMAP does not support tx line interrupt in
>> UART mode. So the latter is not an option.
> 
> Oh, I am sorry, it does support. There is "Supplementary Control
> Register" described in 19.5.1.39

For the moment then, can we add a UART_CAP_SW485 (not exposed to userspace)
that enables this algorithm only for h/w that supports a both-empty interrupt
mode. The probe or driver (ala 8250_omap) would opt-in and configure the h/w much
like the omap-serial driver does now (with the SCR register).

Does that seem like an acceptable compromise?

Regards,
Peter Hurley

PS - I still need to review this series for how the timer logic works esp. wrt
teardown.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1282720

2015-12-03 2:20 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 11/18/2015 02:49 PM, Matwey V. Kornilov wrote:
>> 2015-11-18 22:39 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>> 2015-11-18 21:33 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>> On 11/17/2015 03:20 AM, Matwey V. Kornilov wrote:
>>>>> 2015-11-16 22:18 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>> On 11/14/2015 10:25 AM, One Thousand Gnomes wrote:
> [...]
>>>>>>> It's also not "easy to drop". If it ever goes in we are stuck with a
>>>>>>> pointless impossible to correctly set flag for all eternity.
>>>>>>>
>>>>>>> Please explain the correct setting for this flag when a device driver
>>>>>>> uses hardware or software or a mix according to what the silicon is
>>>>>>> capable of and what values are requested ? How will an application use the
>>>>>>> flag meaningfully. Please explain what will happen if someone discovers a
>>>>>>> silicon bug and in a future 4.x release turns an implementation from
>>>>>>> hardware to software - will they have to lie about the flag to avoid
>>>>>>> breaking their application code - that strikes me as a bad thing.
>>>>>>
>>>>>> The existing driver behavior is already significantly variant and needs
>>>>>> to be converged, which shouldn't be too difficult. Here's a quick summary:
>>>>>>
>>>>>> mcfuart         ignores delay values, delays unsupported
>>>>>> imx             clamps delay values to 0, delays unsupported
>>>>>> atmel           only delay_rts_after_send used; delay_rts_before_send does nothing
>>>>>> 8250_fintek     clamps delay values to 1, unclear if h/w delay is msecs
>>>>>> omap-serial*    software emulation (but tx empty polling not reqd)
>>>>>> lpc18xx-uart    clamps delay_rts_before_send to 0, unsupported
>>>>>>                 clamps delay_rts_after_send to max h/w value
>>>>>> max310x         returns -ERANGE if either delay value > h/w support (15 msecs)
>>>>>> sc16is7xx*      returns -EINVAL if delay_rts_after_send is set
>>>>>> crisv10*        clamps delay_rts_before_send to 1000 msecs
>>>>>>                 ignores delays_rts_after_send (after dma is delayed by 2 * chars)
>>>>>> * implements delay(s) in software
>>>>>>
>>>>>> The omap-serial emulation should not have been merged in its current form.
>>>>>>
>>>>>> IMO the proper driver behavior should be clamp to h/w limit so an application
>>>>>> can determine the maximum delay supported. If a delay is unsupported, it should
>>>>>> be clamped to 0. The application should check the RS485 settings returned by
>>>>>> TIOCSRS485 to determine how the driver set them.
>>>>>> [ Documentation/serial/serial-rs485.txt should suggest/model this action ]
>>>>>
>>>>> But the similar could be true for minimal supported delay. If user
>>>>> requires delay which is less than lower bound, the delay is raised to
>>>>> the lower bound. If user requires delay which is greater than upper
>>>>> bound, the delay is set to the upper bound. Then software
>>>>> implementation could use (tx fifo size / baudrate) as lower bound for
>>>>> delay_after_send.
>>>>
>>>> From the application point-of-view (really the only relevant semantics),
>>>> delay_dts_after_send refers to the number of milliseconds to delay the
>>>> toggle of RTS after the last bit has been _transmitted_.
>
> Is there consensus then about what the semantics of unsupported RS485 delay
> values are? I (or someone else) can trivially add the documentation and
> fixes to the existing in-tree drivers.
>
>
>>>> A couple of possibilities for improving the emulation are:
>>>> 1) Optionally using an HR timer for sub-jiffy turnaround.
>>>> 2) Only supporting 8250-based hardware that can be set to interrupt when
>>>>    both tx fifo and transmitter shift register are empty.
>>>
>>> This is to support the RS485 API with already exists in omap_serrial,
>>> but not in 8250_omap. And OMAP does not support tx line interrupt in
>>> UART mode. So the latter is not an option.
>>
>> Oh, I am sorry, it does support. There is "Supplementary Control
>> Register" described in 19.5.1.39
>
> For the moment then, can we add a UART_CAP_SW485 (not exposed to userspace)
> that enables this algorithm only for h/w that supports a both-empty interrupt
> mode. The probe or driver (ala 8250_omap) would opt-in and configure the h/w much
> like the omap-serial driver does now (with the SCR register).
>
> Does that seem like an acceptable compromise?
>
> Regards,
> Peter Hurley
>
> PS - I still need to review this series for how the timer logic works esp. wrt
> teardown.
>

Dear Peter,

I am working on v4, where I completely redesigned implementation. And
now I think that it is considerably better than v3.
It looks like the following:
https://github.com/matwey/linux/commits/8520_rs485_v4
But it is not ready yet, there is a bug somewhere.

In the v4, each subdriver decides separately if it needs rs485
emulation support. Then it enables it like the following:
https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
Before calling serial8250_rs485_emul_enabled, the driver enables
interrupt on empty shift register (they are always there for omap_).

-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283091

Hi Matwey,

On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
> I am working on v4, where I completely redesigned implementation. And
> now I think that it is considerably better than v3.
> It looks like the following:
> https://github.com/matwey/linux/commits/8520_rs485_v4
> But it is not ready yet, there is a bug somewhere.
> 
> In the v4, each subdriver decides separately if it needs rs485
> emulation support. Then it enables it like the following:
> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
> Before calling serial8250_rs485_emul_enabled, the driver enables
> interrupt on empty shift register (they are always there for omap_).
 
Looks good.

Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
debug effort? DMA adds a completely different tx path.

Also, before submission, please shorten the identifiers. And Greg hates
functions returning bool so just expanded serial8250_rs485_emul_enabled()
inline.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283213

2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> Hi Matwey,
>
> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>> I am working on v4, where I completely redesigned implementation. And
>> now I think that it is considerably better than v3.
>> It looks like the following:
>> https://github.com/matwey/linux/commits/8520_rs485_v4
>> But it is not ready yet, there is a bug somewhere.
>>
>> In the v4, each subdriver decides separately if it needs rs485
>> emulation support. Then it enables it like the following:
>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>> Before calling serial8250_rs485_emul_enabled, the driver enables
>> interrupt on empty shift register (they are always there for omap_).
>
> Looks good.
>
> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
> debug effort? DMA adds a completely different tx path.

Many thanks for the advice. I've just found that the bug is not in my code =)
Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
hangs on open() and the process is in S+ state.

>
> Also, before submission, please shorten the identifiers. And Greg hates
> functions returning bool so just expanded serial8250_rs485_emul_enabled()
> inline.

Am I allowed to use `re' instead of rs485_emul in names?

>
> Regards,
> Peter Hurley
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283293

On 12/03/2015 12:29 PM, Matwey V. Kornilov wrote:
> 2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>> Hi Matwey,
>>
>> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>>> I am working on v4, where I completely redesigned implementation. And
>>> now I think that it is considerably better than v3.
>>> It looks like the following:
>>> https://github.com/matwey/linux/commits/8520_rs485_v4
>>> But it is not ready yet, there is a bug somewhere.
>>>
>>> In the v4, each subdriver decides separately if it needs rs485
>>> emulation support. Then it enables it like the following:
>>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>>> Before calling serial8250_rs485_emul_enabled, the driver enables
>>> interrupt on empty shift register (they are always there for omap_).
>>
>> Looks good.
>>
>> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
>> debug effort? DMA adds a completely different tx path.
> 
> Many thanks for the advice. I've just found that the bug is not in my code =)
> Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
> hangs on open() and the process is in S+ state.

Hmm, that's odd. So

$ stty -a < /dev/ttyS5

hangs if something like below is running?

$ cat > /dev/ttyS5


>> Also, before submission, please shorten the identifiers. And Greg hates
>> functions returning bool so just expanded serial8250_rs485_emul_enabled()
>> inline.
> 
> Am I allowed to use `re' instead of rs485_emul in names?

Long names and constructs tend to obscure the execution flow.
Some of the names could be reduced where the meaning is obvious:

  serial8250_rts_on_send
  serial8250_rts_after_send
  serial8250_handle_start_timer
  serial8250_handle_stop_timer

These two I would inline into their lone call site:

  serial8250_rs485_emul_startup()
  serial8250_rs485_emul_shutdown()

serial8250_rs485_emul_start_tx  => __start_tx_rs485

rs485_emul => sw485/em485/emul485/soft485 ?

Or just rs485 (except for the field name and structs so as not to confuse
it with the port->rs485)

Just my 2¢

Regards,
Peter Hurley


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1284127

2015-12-03 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 12/03/2015 12:29 PM, Matwey V. Kornilov wrote:
>> 2015-12-03 17:41 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> Hi Matwey,
>>>
>>> On 12/03/2015 12:50 AM, Matwey V. Kornilov wrote:
>>>> I am working on v4, where I completely redesigned implementation. And
>>>> now I think that it is considerably better than v3.
>>>> It looks like the following:
>>>> https://github.com/matwey/linux/commits/8520_rs485_v4
>>>> But it is not ready yet, there is a bug somewhere.
>>>>
>>>> In the v4, each subdriver decides separately if it needs rs485
>>>> emulation support. Then it enables it like the following:
>>>> https://github.com/matwey/linux/commit/4455e425fc045713fb921ccec695fe183f1558f0
>>>> Before calling serial8250_rs485_emul_enabled, the driver enables
>>>> interrupt on empty shift register (they are always there for omap_).
>>>
>>> Looks good.
>>>
>>> Are you testing with CONFIG_SERIAL_8250_DMA=n first to simplify the
>>> debug effort? DMA adds a completely different tx path.
>>
>> Many thanks for the advice. I've just found that the bug is not in my code =)
>> Even with pure 4.3.0 I cannot open /dev/ttyS5 more than once. It just
>> hangs on open() and the process is in S+ state.
>
> Hmm, that's odd. So
>
> $ stty -a < /dev/ttyS5
>
> hangs if something like below is running?
>
> $ cat > /dev/ttyS5
>

Nonblocking mode works, blocking mode hands on tty_port_block_til_ready
https://bugzilla.kernel.org/show_bug.cgi?id=108851

>
>>> Also, before submission, please shorten the identifiers. And Greg hates
>>> functions returning bool so just expanded serial8250_rs485_emul_enabled()
>>> inline.

I would like to keep it as API to hide implementation details.
In other words, I don't want to inline it in omap_8250_rs485_config.

>>
>> Am I allowed to use `re' instead of rs485_emul in names?
>
> Long names and constructs tend to obscure the execution flow.
> Some of the names could be reduced where the meaning is obvious:
>
>   serial8250_rts_on_send
>   serial8250_rts_after_send
>   serial8250_handle_start_timer
>   serial8250_handle_stop_timer
>
> These two I would inline into their lone call site:
>
>   serial8250_rs485_emul_startup()
>   serial8250_rs485_emul_shutdown()
>
> serial8250_rs485_emul_start_tx  => __start_tx_rs485
>
> rs485_emul => sw485/em485/emul485/soft485 ?
>
> Or just rs485 (except for the field name and structs so as not to confuse
> it with the port->rs485)
>
> Just my 2¢
>
> Regards,
> Peter Hurley
>
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web