Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Greg Kroah-Hartman]

On Wed, Sep 21, 2022 at 06:39:33AM +0200, Lukas Wunner wrote:
> When a UART port is newly registered, uart_configure_port() seeks to
> deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> However a number of UART drivers interpret a set RTS bit as *assertion*
> instead of deassertion:  Affected drivers include those using
> serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> mctrl_gpio (e.g. imx.c).
> 
> Since the interpretation of the RTS bit is driver-specific, it is not
> suitable as a means to centrally deassert Transmit Enable in the serial
> core.  Instead, the serial core must call on drivers to deassert it in
> their driver-specific way.  One way to achieve that is to call
> ->rs485_config().  It implicitly deasserts Transmit Enable.
> 
> So amend uart_configure_port() and uart_resume_port() to invoke
> uart_rs485_config().  That allows removing calls to uart_rs485_config()
> from drivers' ->probe() hooks and declaring the function static.
> 
> Skip any invocation of ->set_mctrl() if RS485 is enabled.  RS485 has no
> hardware flow control, so the modem control lines are irrelevant and
> need not be touched.  When leaving RS485 mode, reset the modem control
> lines to the state stored in port->mctrl.  That way, UARTs which are
> muxed between RS485 and RS232 transceivers drive the lines correctly
> when switched to RS232.  (serial8250_do_startup() historically raises
> the OUT1 modem signal because otherwise interrupts are not signaled on
> ancient PC UARTs, but I believe that no longer applies to modern,
> RS485-capable UARTs and is thus safe to be skipped.)
> 
> imx.c modifies port->mctrl whenever Transmit Enable is asserted and
> deasserted.  Stop it from doing that so port->mctrl reflects the RS232
> line state.
> 
> 8250_omap.c deasserts Transmit Enable on ->runtime_resume() by calling
> ->set_mctrl().  Because that is now a no-op in RS485 mode, amend the
> function to call serial8250_em485_stop_tx().
> 
> fsl_lpuart.c retrieves and applies the RS485 device tree properties
> after registering the UART port.  Because applying now happens on
> registration in uart_configure_port(), move retrieval of the properties
> ahead of uart_add_one_port().
> 
> Fixes: d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
> Reported-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Tested-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Link: https://lore.kernel.org/all/20220329085050.311408-1-matthias.schiffer@ew.tq-group.com/
> Reported-by: Roosen Henri <Henri.Roosen@ginzinger.com>
> Link: https://lore.kernel.org/all/8f538a8903795f22f9acc94a9a31b03c9c4ccacb.camel@ginzinger.com/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.14+
> ---
>  v1 -> v2:
>  Deassert RTS in serial8250_em485_init() only if no transmission is
>  currently ongoing (Ilpo)
> 
>  Based on v6.0-rc3 + this dependency:
>  https://lore.kernel.org/linux-serial/72fb646c1b0b11c989850c55f52f9ff343d1b2fa.1662884345.git.lukas@wunner.de/

This message never made it to lore.kernel.org, so I can't seem to apply
it using `b4`.

Can you resend it so that it does make it to the public archives?

thanks,

greg k-h

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Lukas Wunner]

On Thu, Sep 22, 2022 at 04:43:51PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 21, 2022 at 06:39:33AM +0200, Lukas Wunner wrote:
> > When a UART port is newly registered, uart_configure_port() seeks to
> > deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> > However a number of UART drivers interpret a set RTS bit as *assertion*
> > instead of deassertion:  Affected drivers include those using
> > serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> > mctrl_gpio (e.g. imx.c).
> > 
> > Since the interpretation of the RTS bit is driver-specific, it is not
> > suitable as a means to centrally deassert Transmit Enable in the serial
> > core.  Instead, the serial core must call on drivers to deassert it in
> > their driver-specific way.  One way to achieve that is to call
> > ->rs485_config().  It implicitly deasserts Transmit Enable.
> > 
> > So amend uart_configure_port() and uart_resume_port() to invoke
> > uart_rs485_config().  That allows removing calls to uart_rs485_config()
> > from drivers' ->probe() hooks and declaring the function static.
[...]
> 
> This message never made it to lore.kernel.org, so I can't seem to apply
> it using `b4`.
> 
> Can you resend it so that it does make it to the public archives?

Yes, both v1 and v2 didn't make it to the mailing list archive.
My suspicion is that the Cc: line was probably too long.

I resent as v3 with only you in To: and the mailing list in Cc: and
this time it went through:

https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663860626.git.lukas@wunner.de/

On the bright side, v2 contained an embarrassing checkpatch issue
(superfluous newline) and resending as v3 provided a welcome
opportunity to fix that. :)

Thanks,

Lukas

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Greg Kroah-Hartman]

On Thu, Sep 22, 2022 at 05:43:53PM +0200, Lukas Wunner wrote:
> On Thu, Sep 22, 2022 at 04:43:51PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 21, 2022 at 06:39:33AM +0200, Lukas Wunner wrote:
> > > When a UART port is newly registered, uart_configure_port() seeks to
> > > deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> > > However a number of UART drivers interpret a set RTS bit as *assertion*
> > > instead of deassertion:  Affected drivers include those using
> > > serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> > > mctrl_gpio (e.g. imx.c).
> > > 
> > > Since the interpretation of the RTS bit is driver-specific, it is not
> > > suitable as a means to centrally deassert Transmit Enable in the serial
> > > core.  Instead, the serial core must call on drivers to deassert it in
> > > their driver-specific way.  One way to achieve that is to call
> > > ->rs485_config().  It implicitly deasserts Transmit Enable.
> > > 
> > > So amend uart_configure_port() and uart_resume_port() to invoke
> > > uart_rs485_config().  That allows removing calls to uart_rs485_config()
> > > from drivers' ->probe() hooks and declaring the function static.
> [...]
> > 
> > This message never made it to lore.kernel.org, so I can't seem to apply
> > it using `b4`.
> > 
> > Can you resend it so that it does make it to the public archives?
> 
> Yes, both v1 and v2 didn't make it to the mailing list archive.
> My suspicion is that the Cc: line was probably too long.
> 
> I resent as v3 with only you in To: and the mailing list in Cc: and
> this time it went through:
> 
> https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663860626.git.lukas@wunner.de/
> 
> On the bright side, v2 contained an embarrassing checkpatch issue
> (superfluous newline) and resending as v3 provided a welcome
> opportunity to fix that. :)

v3 did not have a changelog :(

v4?

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Lukas Wunner]

On Thu, Sep 22, 2022 at 06:06:56PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 22, 2022 at 05:43:53PM +0200, Lukas Wunner wrote:
> > On Thu, Sep 22, 2022 at 04:43:51PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 21, 2022 at 06:39:33AM +0200, Lukas Wunner wrote:
> > > > When a UART port is newly registered, uart_configure_port() seeks to
> > > > deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> > > > However a number of UART drivers interpret a set RTS bit as *assertion*
> > > > instead of deassertion:  Affected drivers include those using
> > > > serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> > > > mctrl_gpio (e.g. imx.c).
> > > > 
> > > > Since the interpretation of the RTS bit is driver-specific, it is not
> > > > suitable as a means to centrally deassert Transmit Enable in the serial
> > > > core.  Instead, the serial core must call on drivers to deassert it in
> > > > their driver-specific way.  One way to achieve that is to call
> > > > ->rs485_config().  It implicitly deasserts Transmit Enable.
> > > > 
> > > > So amend uart_configure_port() and uart_resume_port() to invoke
> > > > uart_rs485_config().  That allows removing calls to uart_rs485_config()
> > > > from drivers' ->probe() hooks and declaring the function static.
> > [...]
> > > 
> > > This message never made it to lore.kernel.org, so I can't seem to apply
> > > it using `b4`.
> > > 
> > > Can you resend it so that it does make it to the public archives?
> > 
> > Yes, both v1 and v2 didn't make it to the mailing list archive.
> > My suspicion is that the Cc: line was probably too long.
> > 
> > I resent as v3 with only you in To: and the mailing list in Cc: and
> > this time it went through:
> > 
> > https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663860626.git.lukas@wunner.de/
> > 
> > On the bright side, v2 contained an embarrassing checkpatch issue
> > (superfluous newline) and resending as v3 provided a welcome
> > opportunity to fix that. :)
> 
> v3 did not have a changelog :(
> 
> v4?

Well, the changelog is above.  (Only the superfluous newline was removed
in v3 vis-à-vis v2.)

Here's a v4 with full changelog:

https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663863805.git.lukas@wunner.de/

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Matthias Schiffer]

On Thu, 2022-09-22 at 18:30 +0200, Lukas Wunner wrote:
> On Thu, Sep 22, 2022 at 06:06:56PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 22, 2022 at 05:43:53PM +0200, Lukas Wunner wrote:
> > > On Thu, Sep 22, 2022 at 04:43:51PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 21, 2022 at 06:39:33AM +0200, Lukas Wunner wrote:
> > > > > When a UART port is newly registered, uart_configure_port() seeks to
> > > > > deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> > > > > However a number of UART drivers interpret a set RTS bit as *assertion*
> > > > > instead of deassertion:  Affected drivers include those using
> > > > > serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> > > > > mctrl_gpio (e.g. imx.c).
> > > > > 
> > > > > Since the interpretation of the RTS bit is driver-specific, it is not
> > > > > suitable as a means to centrally deassert Transmit Enable in the serial
> > > > > core.  Instead, the serial core must call on drivers to deassert it in
> > > > > their driver-specific way.  One way to achieve that is to call
> > > > > ->rs485_config().  It implicitly deasserts Transmit Enable.
> > > > > 
> > > > > So amend uart_configure_port() and uart_resume_port() to invoke
> > > > > uart_rs485_config().  That allows removing calls to uart_rs485_config()
> > > > > from drivers' ->probe() hooks and declaring the function static.
> > > [...]
> > > > This message never made it to lore.kernel.org, so I can't seem to apply
> > > > it using `b4`.
> > > > 
> > > > Can you resend it so that it does make it to the public archives?
> > > 
> > > Yes, both v1 and v2 didn't make it to the mailing list archive.
> > > My suspicion is that the Cc: line was probably too long.
> > > 
> > > I resent as v3 with only you in To: and the mailing list in Cc: and
> > > this time it went through:
> > > 
> > > https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663860626.git.lukas@wunner.de/
> > > 
> > > On the bright side, v2 contained an embarrassing checkpatch issue
> > > (superfluous newline) and resending as v3 provided a welcome
> > > opportunity to fix that. :)
> > 
> > v3 did not have a changelog :(
> > 
> > v4?
> 
> Well, the changelog is above.  (Only the superfluous newline was removed
> in v3 vis-à-vis v2.)
> 
> Here's a v4 with full changelog:
> 
> https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663863805.git.lukas@wunner.de/

Hi Lukas,

I've noticed that this patch (well, the version that was applied to
tty.git) also changed the setting of the DTR flag in the MCR register.
Without your patch, I can see that the values passed to
serial8250_out_MCR() alternate between 0x03 and 0x01 when switching
between tx and rx, but with your patch, the values become 0x02 and
0x00.

I'm not sure if setups RS485 exist where the DTR flag is relevant, but
as this was not mentioned in the commit message, I suspect that the
change might have been unintended.

Re: [PATCH v2] serial: Deassert Transmit Enable on probe in driver-specific way

[Lukas Wunner]

On Fri, Oct 07, 2022 at 01:34:19PM +0200, Matthias Schiffer wrote:
> On Thu, 2022-09-22 at 18:30 +0200, Lukas Wunner wrote:
> > Here's a v4 with full changelog:
> > 
> > https://lore.kernel.org/linux-serial/2de36eba3fbe11278d5002e4e501afe0ceaca039.1663863805.git.lukas@wunner.de/
> 
> I've noticed that this patch (well, the version that was applied to
> tty.git) also changed the setting of the DTR flag in the MCR register.
> Without your patch, I can see that the values passed to
> serial8250_out_MCR() alternate between 0x03 and 0x01 when switching
> between tx and rx, but with your patch, the values become 0x02 and
> 0x00.
> 
> I'm not sure if setups RS485 exist where the DTR flag is relevant, but
> as this was not mentioned in the commit message, I suspect that the
> change might have been unintended.

That's intentional and documented in the following paragraph of the
commit message:

  Skip any invocation of ->set_mctrl() if RS485 is enabled.  RS485 has no
  hardware flow control, so the modem control lines are irrelevant and
  need not be touched.  When leaving RS485 mode, reset the modem control
  lines to the state stored in port->mctrl.  That way, UARTs which are
  muxed between RS485 and RS232 transceivers drive the lines correctly
  when switched to RS232.  (serial8250_do_startup() historically raises
  the OUT1 modem signal because otherwise interrupts are not signaled on
  ancient PC UARTs, but I believe that no longer applies to modern,
  RS485-capable UARTs and is thus safe to be skipped.)

I think that the Siemens IOT20x0 series muxes the UART between RS232
and RS485 transceivers, though I'm not sure if that applies both to
the contemporary AM6548-based IOT2050 and to the older Intel-based
machines.

In RS485 mode, the DTR signal should be completely irrelevant as
the transceiver is only attached to RX, TX and RTS pins.

When switching from RS485 to RS232 mode, the patch ensures that the
modem signals are reset to what's been saved in port->mctrl.  So DTR
will be raised once the UART is switched to RS232.

Thanks,

Lukas