On Fri, Sep 15, 2017 at 05:32:16PM +0200, Clemens Gruber wrote:
> On Fri, Sep 15, 2017 at 04:44:56PM +0200, Uwe Kleine-König wrote:
> > On Fri, Sep 15, 2017 at 02:56:35PM +0200, Clemens Gruber wrote:
> > > On Thu, Sep 14, 2017 at 01:11:33PM +0100, Ian Jamison wrote:
> > > > UART core function uart_update_mctrl relies on a cached value of
> > > > modem control lines. This was used but not updated by local RTS
> > > > control functions within imx.c. These are used for RS485 line
> > > > driver enable signalling. Having an out-of-date value in the cached
> > > > mctrl can result in the transmitter being enabled when it shouldn't
> > > > be.
> > > >
> > > > Fix this by updating the mctrl value before applying it.
> > > >
> > > > Cc: Uwe-Kleine König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Cc: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
> > > > Cc: Fabio Estevam <festevam@xxxxxxxxx>
> > > > Cc: Fabio Estevam <fabio.estevam@xxxxxxx>
> > > > Signed-off-by: Ian Jamison <ian.dev@xxxxxxxxxx>
> > > > ---
> > > > Compile tested only, since my devboard uses a monostable off the TXD
> > > > line to enable the transmitter.
> > > >
> > > > Resending to CC Fabio and Clemens.
> > > >
> > > > drivers/tty/serial/imx.c | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index dfeff3951f93..3657d745e90f 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -334,7 +334,8 @@ static void imx_port_rts_active(struct imx_port *sport, unsigned long *ucr2)
> > > > {
> > > > *ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
> > > >
> > > > - mctrl_gpio_set(sport->gpios, sport->port.mctrl | TIOCM_RTS);
> > > > + sport->port.mctrl |= TIOCM_RTS;
> > > > + mctrl_gpio_set(sport->gpios, sport->port.mctrl);
> > > > }
> > > >
> > > > static void imx_port_rts_inactive(struct imx_port *sport, unsigned long *ucr2)
> > > > @@ -342,7 +343,8 @@ static void imx_port_rts_inactive(struct imx_port *sport, unsigned long *ucr2)
> > > > *ucr2 &= ~UCR2_CTSC;
> > > > *ucr2 |= UCR2_CTS;
> > > >
> > > > - mctrl_gpio_set(sport->gpios, sport->port.mctrl & ~TIOCM_RTS);
> > > > + sport->port.mctrl &= ~TIOCM_RTS;
> > > > + mctrl_gpio_set(sport->gpios, sport->port.mctrl);
> > > > }
> > > >
> > > > static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
> > > > --
> > > > 2.14.1
> > > >
> > >
> > > Looks good to me. I tested RS-485 communications with this patch applied
> > > between an i.MX6Q board and my host PC (with a RS-485-to-232 converter).
> > >
> > > Tested-by: Clemens Gruber <clemens.gruber@xxxxxxxxxxxx>
> >
> > Did it actually repair someting? Do you use it in combination with my
> > patch that masked RTS from uart_dtr_rts? Do you use a gpio for RTS or
> > the native function?
>
> Normally, I use the native CTS_B pin, but to test this patch, I remuxed
> the pin to GPIO6_IO3 and added rts-gpios to the uart in the DT.
> Then I did a few tests without the patch but I did not experience any
> problems. The transmitter was only active when it should be.
> Applying the patch did not repair anything for me, but it also did not
> do any harm. Everything still works. Ian wrote "Compile tested only", so
> I made sure it did not break anything for me.
> No, I don't think I have your patch in my inbox. You can send it to me
> if you want me to test it.
>
> > I have this change in my wip state, too, and it improves the situation,
> > but it's not working compeletely yet.
>
> Any idea how to reproduce it? Can you be more specific about the
> situation with and without this patch?
Something that you might be able to reproduce on your side:
enable gpio tracing (i.e.
echo 1 > /sys/kernel/debug/tracing/events/gpio/enable)
and then do
cat /dev/ttymxcX
. For me this makes RTS toggle (while IMHO it shouldn't).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
