[5/8] dt_bindings: rs485: Correct delay values

Commit Message

Lino Sanfilippo June 22, 2022, 3:46 p.m. UTC

From: Lino Sanfilippo <[e-mail]>

The maximum allowed delay for RTS before and RTS after send is 100 ms.
Adjust the documentation accordingly.

Signed-off-by: Lino Sanfilippo <[e-mail]>
---
 Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko June 23, 2022, 4:29 p.m. UTC | #1

On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[e-mail]>
> 
> The maximum allowed delay for RTS before and RTS after send is 100 ms.
> Adjust the documentation accordingly.


Is it only documentation issue? If the code allows this to be set higher
than 100, we may not change the documentation since this an ABI (from
firmware <--> kernel perspective) we need to support old variants.

If the above is true and limit is dictated by the spec, we may issue a
warning in the code and drop it to the allowed maximum, otherwise we
can't do much here.

Lino Sanfilippo June 23, 2022, 8:17 p.m. UTC | #2

On 23.06.22 at 18:29, Andy Shevchenko wrote:
> On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <[e-mail]>
>>
>> The maximum allowed delay for RTS before and RTS after send is 100 ms.
>> Adjust the documentation accordingly.
>
>
> Is it only documentation issue? If the code allows this to be set higher
> than 100, we may not change the documentation since this an ABI (from
> firmware <--> kernel perspective) we need to support old variants.
>

Well currently the documentation claims that a maximum of 1000 msecs is allowed but
nothing actually checks the values read from device tree/ACPI and so it is possible
to set much higher values (note that the UART drivers dont check the delays read from
DT/ACPI either, the only exception I found is max310x which clamps it to 15 ms).

We already have a maximum of 100 ms defined for RTS delays set via TIOCSRS485. To be
consistent with TIOCSRS485 the same limit is used for DT/ACPI values in this patch.

I am aware that this changes the firmware/kernel ABI. But we had a similar situation when
the sanity checks for TIOCSRS485 were introduced
(see https://lore.kernel.org/all/[e-mail]/)
since before we did not have those limits for all drivers (some drivers clamped the
values itself but many did not care).
Furthermore 100 ms is already a very high value for RTS delays (which are usually rather
in usecs range). So IMHO the risk is very low to break anything when values are clamped
that are higher than that.


Regards,
Lino

Ilpo Järvinen June 25, 2022, 9:54 a.m. UTC | #3

On Thu, 23 Jun 2022, Lino Sanfilippo wrote:

> On 23.06.22 at 18:29, Andy Shevchenko wrote:
> > On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
> >> From: Lino Sanfilippo <[e-mail]>
> >>
> >> The maximum allowed delay for RTS before and RTS after send is 100 ms.
> >> Adjust the documentation accordingly.
> >
> >
> > Is it only documentation issue? If the code allows this to be set higher
> > than 100, we may not change the documentation since this an ABI (from
> > firmware <--> kernel perspective) we need to support old variants.
> >
> 
> Well currently the documentation claims that a maximum of 1000 msecs is allowed but
> nothing actually checks the values read from device tree/ACPI and so it is possible
> to set much higher values (note that the UART drivers dont check the delays read from
> DT/ACPI either, the only exception I found is max310x which clamps it to 15 ms).
> 
> We already have a maximum of 100 ms defined for RTS delays set via TIOCSRS485. To be
> consistent with TIOCSRS485 the same limit is used for DT/ACPI values in this patch.
> 
> I am aware that this changes the firmware/kernel ABI. But we had a similar situation when
> the sanity checks for TIOCSRS485 were introduced
> (see https://lore.kernel.org/all/[e-mail]/)
> since before we did not have those limits for all drivers (some drivers clamped the
> values itself but many did not care).
> Furthermore 100 ms is already a very high value for RTS delays (which are usually rather
> in usecs range). So IMHO the risk is very low to break anything when values are clamped
> that are higher than that.

Did you see this development direction (from Lukas):

https://lore.kernel.org/linux-serial/[e-mail]/

?

Effectively, he wants to making a compat threshold at 1msec and beyond 
that the input value would be interpreted as nsecs.

Ilpo Järvinen June 27, 2022, 9:23 a.m. UTC | #4

On Sat, 25 Jun 2022, Ilpo Järvinen wrote:

> On Thu, 23 Jun 2022, Lino Sanfilippo wrote:
> 
> > On 23.06.22 at 18:29, Andy Shevchenko wrote:
> > > On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
> > >> From: Lino Sanfilippo <[e-mail]>
> > >>
> > >> The maximum allowed delay for RTS before and RTS after send is 100 ms.
> > >> Adjust the documentation accordingly.
> > >
> > >
> > > Is it only documentation issue? If the code allows this to be set higher
> > > than 100, we may not change the documentation since this an ABI (from
> > > firmware <--> kernel perspective) we need to support old variants.
> > >
> > 
> > Well currently the documentation claims that a maximum of 1000 msecs is allowed but
> > nothing actually checks the values read from device tree/ACPI and so it is possible
> > to set much higher values (note that the UART drivers dont check the delays read from
> > DT/ACPI either, the only exception I found is max310x which clamps it to 15 ms).
> > 
> > We already have a maximum of 100 ms defined for RTS delays set via TIOCSRS485. To be
> > consistent with TIOCSRS485 the same limit is used for DT/ACPI values in this patch.
> > 
> > I am aware that this changes the firmware/kernel ABI. But we had a similar situation when
> > the sanity checks for TIOCSRS485 were introduced
> > (see https://lore.kernel.org/all/[e-mail]/)
> > since before we did not have those limits for all drivers (some drivers clamped the
> > values itself but many did not care).
> > Furthermore 100 ms is already a very high value for RTS delays (which are usually rather
> > in usecs range). So IMHO the risk is very low to break anything when values are clamped
> > that are higher than that.
> 
> Did you see this development direction (from Lukas):
> 
> https://lore.kernel.org/linux-serial/[e-mail]/
> 
> ?
> 
> Effectively, he wants to making a compat threshold at 1msec and beyond 
> that the input value would be interpreted as nsecs.

I was thinking this more the other day and came up with the idea of adding
SER_RS485_DELAY_RTS_NSEC and SER_RS485_DELAY_RTS_MSEC flags instead of
magic threshold and deprecate specifying those delays w/o either flag. 
That way we'd not need to change behavior and we provide an easy way to 
keep the delay in msec if somebody really wants (just for the sake of 
getting rid of the warning).

Andy Shevchenko June 28, 2022, 10:03 a.m. UTC | #5

On Thu, Jun 23, 2022 at 10:17:06PM +0200, Lino Sanfilippo wrote:
> On 23.06.22 at 18:29, Andy Shevchenko wrote:
> > On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
> >>
> >> The maximum allowed delay for RTS before and RTS after send is 100 ms.
> >> Adjust the documentation accordingly.
> >
> > Is it only documentation issue? If the code allows this to be set higher
> > than 100, we may not change the documentation since this an ABI (from
> > firmware <--> kernel perspective) we need to support old variants.
> 
> Well currently the documentation claims that a maximum of 1000 msecs is allowed but
> nothing actually checks the values read from device tree/ACPI and so it is possible
> to set much higher values (note that the UART drivers dont check the delays read from
> DT/ACPI either, the only exception I found is max310x which clamps it to 15 ms).
> 
> We already have a maximum of 100 ms defined for RTS delays set via TIOCSRS485. To be
> consistent with TIOCSRS485 the same limit is used for DT/ACPI values in this patch.
> 
> I am aware that this changes the firmware/kernel ABI. But we had a similar situation when
> the sanity checks for TIOCSRS485 were introduced
> (see https://lore.kernel.org/all/[e-mail]/)
> since before we did not have those limits for all drivers (some drivers clamped the
> values itself but many did not care).
> Furthermore 100 ms is already a very high value for RTS delays (which are usually rather
> in usecs range). So IMHO the risk is very low to break anything when values are clamped
> that are higher than that.

You need to elaborate all this in the commit message to justify the change.

Lino Sanfilippo June 29, 2022, 11:50 p.m. UTC | #6

On 28.06.22 12:03, Andy Shevchenko wrote:
> On Thu, Jun 23, 2022 at 10:17:06PM +0200, Lino Sanfilippo wrote:
>> On 23.06.22 at 18:29, Andy Shevchenko wrote:
>>> On Wed, Jun 22, 2022 at 05:46:56PM +0200, Lino Sanfilippo wrote:
>>>>
>>>> The maximum allowed delay for RTS before and RTS after send is 100 ms.
>>>> Adjust the documentation accordingly.
>>>
>>> Is it only documentation issue? If the code allows this to be set higher
>>> than 100, we may not change the documentation since this an ABI (from
>>> firmware <--> kernel perspective) we need to support old variants.
>>
>> Well currently the documentation claims that a maximum of 1000 msecs is allowed but
>> nothing actually checks the values read from device tree/ACPI and so it is possible
>> to set much higher values (note that the UART drivers dont check the delays read from
>> DT/ACPI either, the only exception I found is max310x which clamps it to 15 ms).
>>
>> We already have a maximum of 100 ms defined for RTS delays set via TIOCSRS485. To be
>> consistent with TIOCSRS485 the same limit is used for DT/ACPI values in this patch.
>>
>> I am aware that this changes the firmware/kernel ABI. But we had a similar situation when
>> the sanity checks for TIOCSRS485 were introduced
>> (see https://lore.kernel.org/all/[e-mail]/)
>> since before we did not have those limits for all drivers (some drivers clamped the
>> values itself but many did not care).
>> Furthermore 100 ms is already a very high value for RTS delays (which are usually rather
>> in usecs range). So IMHO the risk is very low to break anything when values are clamped
>> that are higher than that.
>
> You need to elaborate all this in the commit message to justify the change.
>

OK, I see. I will rewrite the commit message then to hopefully make the rationale behind
the time reduction more clear.

Thanks,
Lino

Patch

diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index f2c9c9fe6aa7..90a1bab40f05 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -22,12 +22,12 @@  properties:
         - description: Delay between rts signal and beginning of data sent in
             milliseconds. It corresponds to the delay before sending data.
           default: 0
-          maximum: 1000
+          maximum: 100
         - description: Delay between end of data sent and rts signal in milliseconds.
             It corresponds to the delay after sending data and actual release
             of the line.
           default: 0
-          maximum: 1000
+          maximum: 100
 
   rs485-rts-active-low:
     description: drive RTS low when sending (default is high).