Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Martyn Welch]

Hi

On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-König wrote:
> Cc += Clemens Gruber + Fabio Estevam
> 
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han 
> > 
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> > 
> > Signed-off-by: Romain Perier 
> 
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
> 

This patch was broken out from a larger one written by Nandor, who is
happy for me to add his sob.

> > ---
> >  drivers/tty/serial/imx.c | 5 -
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >  
> > -   /* clear UCR2 */
> > -   temp = readl(sport->port.membase + UCR2);
> > -   temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -   writel(temp, sport->port.membase + UCR2);
> > -
> 
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
> 
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
> 

I've been giving the RTS/CTS lines a bit of a kick with (and without) this
patch and I'm seeing what I'd expect to see on the CTS pin (the Wandboard
I'm using runs this in DCE mode even though it really should be in DTE
mode, hey ho). Using a little test app I've found/modified (listed below),
the CTS line can be (de)asserted whilst the device is open and the line
gets deasserted when the device closes. I have tested this port both when
acting as a console (and thus with DMA turned off) and when not used as a
console, with DMA enabled (confirmed with existing debug in driver).

This matches the behaviour of a FTDI based debug board that I've also
tried (in this case I looked at the RTS line as the device is running as a
DTE).

On my PC the same test app sets the RTS line (the serial port running as a
DTE, 8250_pnp driver) results in the CTS line being set appropriately as
well. It also stays in that state even after the serial device is closed,
this does seem right either but there you go.

With regards to the operation of the CTS/RTS line when twiddling it via
the ioctl, I think the behaviour of the IMX/FTDI is the expected one. Is
that the case?

Which I guess brings us to the lines above.

When running as an RS232 port (i.e. not rs485) the driver is using the
automatic CTSC control based on a rxFIFO watermark unless the state of the
CTS line is explictly set via an ioctl call. As such, unless I'm missing
something, the rxFIFO (and thus the automatic CTS control) is independent
of whether the DMA is running or not and thus this section looks wrong or
is at the very least in the wrong place.

Have I misunderstood something?

IIRC, the timing of DMA being enabled and disabled was changed reasonably
recently did that fix the #CTS issue possibly?

Martyn



Test app:

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

static struct termios oldterminfo;


void closeserial(int fd)
{
tcsetattr(fd, TCSANOW, );
if (close(fd) < 0)
perror("closeserial()");
}


int openserial(char *devicename)
{
int fd;
struct termios attr;

if ((fd = open(devicename, O_RDWR)) == -1) {
perror("openserial(): open()");
return 0;
}
if (tcgetattr(fd, ) == -1) {
perror("openserial(): tcgetattr()");
return 0;
}
attr = oldterminfo;
attr.c_cflag |= CRTSCTS | CLOCAL;
attr.c_oflag = 0;
if (tcflush(fd, TCIOFLUSH) == -1) {
perror("openserial(): tcflush()");
return 0;
}
if (tcsetattr(fd, TCSANOW, ) == -1) {
perror("initserial(): tcsetattr()");
return 0;
}
return fd;
}


int setRTS(int fd, int level)
{
int status;

if (ioctl(fd, TIOCMGET, ) == -1) {
perror("setRTS(): TIOCMGET");
return 0;
}
if (level)
status |= TIOCM_RTS;
else
status &= ~TIOCM_RTS;
if (ioctl(fd, TIOCMSET, ) == -1) {
perror("setRTS(): TIOCMSET");
return 0;
}
return 1;
}


int main(int argc, char *argv[])
{
int fd;
bool loop = true;
int state = 1;
int got;


fd = openserial(argv[1]);
if (!fd) {
fprintf(stderr, "Error while initializing %s.\n", argv[1]);
return 1;
}


while(loop) {
printf("Switching RTS %s\n", state ? "on" : "off");
setRTS(fd, state);

state = (++state) % 2;

got = getchar();
if (got == 'q')
break;
}
closeserial(fd);
return 0;
}

Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Martyn Welch]

Hi

On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-König wrote:
> Cc += Clemens Gruber + Fabio Estevam
> 
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han 
> > 
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> > 
> > Signed-off-by: Romain Perier 
> 
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
> 

This patch was broken out from a larger one written by Nandor, who is
happy for me to add his sob.

> > ---
> >  drivers/tty/serial/imx.c | 5 -
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >  
> > -   /* clear UCR2 */
> > -   temp = readl(sport->port.membase + UCR2);
> > -   temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -   writel(temp, sport->port.membase + UCR2);
> > -
> 
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
> 
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
> 

I've been giving the RTS/CTS lines a bit of a kick with (and without) this
patch and I'm seeing what I'd expect to see on the CTS pin (the Wandboard
I'm using runs this in DCE mode even though it really should be in DTE
mode, hey ho). Using a little test app I've found/modified (listed below),
the CTS line can be (de)asserted whilst the device is open and the line
gets deasserted when the device closes. I have tested this port both when
acting as a console (and thus with DMA turned off) and when not used as a
console, with DMA enabled (confirmed with existing debug in driver).

This matches the behaviour of a FTDI based debug board that I've also
tried (in this case I looked at the RTS line as the device is running as a
DTE).

On my PC the same test app sets the RTS line (the serial port running as a
DTE, 8250_pnp driver) results in the CTS line being set appropriately as
well. It also stays in that state even after the serial device is closed,
this does seem right either but there you go.

With regards to the operation of the CTS/RTS line when twiddling it via
the ioctl, I think the behaviour of the IMX/FTDI is the expected one. Is
that the case?

Which I guess brings us to the lines above.

When running as an RS232 port (i.e. not rs485) the driver is using the
automatic CTSC control based on a rxFIFO watermark unless the state of the
CTS line is explictly set via an ioctl call. As such, unless I'm missing
something, the rxFIFO (and thus the automatic CTS control) is independent
of whether the DMA is running or not and thus this section looks wrong or
is at the very least in the wrong place.

Have I misunderstood something?

IIRC, the timing of DMA being enabled and disabled was changed reasonably
recently did that fix the #CTS issue possibly?

Martyn



Test app:

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

static struct termios oldterminfo;


void closeserial(int fd)
{
tcsetattr(fd, TCSANOW, );
if (close(fd) < 0)
perror("closeserial()");
}


int openserial(char *devicename)
{
int fd;
struct termios attr;

if ((fd = open(devicename, O_RDWR)) == -1) {
perror("openserial(): open()");
return 0;
}
if (tcgetattr(fd, ) == -1) {
perror("openserial(): tcgetattr()");
return 0;
}
attr = oldterminfo;
attr.c_cflag |= CRTSCTS | CLOCAL;
attr.c_oflag = 0;
if (tcflush(fd, TCIOFLUSH) == -1) {
perror("openserial(): tcflush()");
return 0;
}
if (tcsetattr(fd, TCSANOW, ) == -1) {
perror("initserial(): tcsetattr()");
return 0;
}
return fd;
}


int setRTS(int fd, int level)
{
int status;

if (ioctl(fd, TIOCMGET, ) == -1) {
perror("setRTS(): TIOCMGET");
return 0;
}
if (level)
status |= TIOCM_RTS;
else
status &= ~TIOCM_RTS;
if (ioctl(fd, TIOCMSET, ) == -1) {
perror("setRTS(): TIOCMSET");
return 0;
}
return 1;
}


int main(int argc, char *argv[])
{
int fd;
bool loop = true;
int state = 1;
int got;


fd = openserial(argv[1]);
if (!fd) {
fprintf(stderr, "Error while initializing %s.\n", argv[1]);
return 1;
}


while(loop) {
printf("Switching RTS %s\n", state ? "on" : "off");
setRTS(fd, state);

state = (++state) % 2;

got = getchar();
if (got == 'q')
break;
}
closeserial(fd);
return 0;
}

Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Clemens Gruber]

Hi,

On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-König wrote:
> Cc += Clemens Gruber + Fabio Estevam
> 
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han 
> > 
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> > 
> > Signed-off-by: Romain Perier 
> 
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
> 
> > ---
> >  drivers/tty/serial/imx.c | 5 -
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >  
> > -   /* clear UCR2 */
> > -   temp = readl(sport->port.membase + UCR2);
> > -   temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -   writel(temp, sport->port.membase + UCR2);
> > -
> 
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
> 
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
> 
> @Clemens: maybe this patch makes a relevant difference when the port is
> operated in rs485 mode. Do you care to test?

I just finished testing it. The results are about the same as with v1 of
this patch series:

Applying v2 of patch 1/6, 2/6 and 3/6 (or even 3/6 alone) does not fix
the RS-485 DMA bug. It behaves exactly the same as without these
patches, meaning the whole xmit circ_buf (UART_XMIT_SIZE bytes) is sent
out, as seen in the logic analyzer screenshots from my first bug report:
https://pqgruber.com/rs485_results.png

Applying the whole series does make a (small) difference though:
The first few transmissions after a fresh boot work correctly! However,
after a few transmissions, characters are sent out twice and longer
transmissions are garbled, although not in the same way as before. The
whole circ_buf is no longer sent out.

With all patches from this series applied, I see the following on the
logic analyzer, when calling "echo Test > /dev/ttymxc4":
https://pqgruber.com/rs485txtest.png
(This pattern is not always the same, sometimes it is TeTesstt\n\n,
sometimes TeTesst\nt\n or TeTsestt\n\n and so on)

This behavior is reproducible on i.MX6Q and i.MX6D, not on i.MX6S/etc.,
I therefore assume that it is a SMP-/locking-related problem.

I will try to debug this further, and verify if - with this series
applied - xmit->tail is still jumping over xmit->head. And when exactly
this is happening.

Help is much appreciated!

Best regards,
Clemens

Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Clemens Gruber]

Hi,

On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-König wrote:
> Cc += Clemens Gruber + Fabio Estevam
> 
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han 
> > 
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> > 
> > Signed-off-by: Romain Perier 
> 
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
> 
> > ---
> >  drivers/tty/serial/imx.c | 5 -
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >  
> > -   /* clear UCR2 */
> > -   temp = readl(sport->port.membase + UCR2);
> > -   temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -   writel(temp, sport->port.membase + UCR2);
> > -
> 
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
> 
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
> 
> @Clemens: maybe this patch makes a relevant difference when the port is
> operated in rs485 mode. Do you care to test?

I just finished testing it. The results are about the same as with v1 of
this patch series:

Applying v2 of patch 1/6, 2/6 and 3/6 (or even 3/6 alone) does not fix
the RS-485 DMA bug. It behaves exactly the same as without these
patches, meaning the whole xmit circ_buf (UART_XMIT_SIZE bytes) is sent
out, as seen in the logic analyzer screenshots from my first bug report:
https://pqgruber.com/rs485_results.png

Applying the whole series does make a (small) difference though:
The first few transmissions after a fresh boot work correctly! However,
after a few transmissions, characters are sent out twice and longer
transmissions are garbled, although not in the same way as before. The
whole circ_buf is no longer sent out.

With all patches from this series applied, I see the following on the
logic analyzer, when calling "echo Test > /dev/ttymxc4":
https://pqgruber.com/rs485txtest.png
(This pattern is not always the same, sometimes it is TeTesstt\n\n,
sometimes TeTesst\nt\n or TeTsestt\n\n and so on)

This behavior is reproducible on i.MX6Q and i.MX6D, not on i.MX6S/etc.,
I therefore assume that it is a SMP-/locking-related problem.

I will try to debug this further, and verify if - with this series
applied - xmit->tail is still jumping over xmit->head. And when exactly
this is happening.

Help is much appreciated!

Best regards,
Clemens

Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Uwe Kleine-König]

Cc += Clemens Gruber + Fabio Estevam

On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> From: Nandor Han 
> 
> CTSC and CTS are not related to DMA and might add
> disruption in some cases.
> 
> Signed-off-by: Romain Perier 

If it was Nandor Han who created this patch, it would be great to get
his sob. If it was you, drop the From: line above.

> ---
>  drivers/tty/serial/imx.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5291b86..dd3ebb4 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
>   imx_stop_rx_dma(sport);
>   imx_stop_tx_dma(sport);
>  
> - /* clear UCR2 */
> - temp = readl(sport->port.membase + UCR2);
> - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> - writel(temp, sport->port.membase + UCR2);
> -

Before this patch imx_disable_dma resulted in the #CTS pin being high
(inactive).

Does this qualify as a fix? If so, you should sort this patch to the
beginning of the series. Did you do test this patch and its effects
separately?

@Clemens: maybe this patch makes a relevant difference when the port is
operated in rs485 mode. Do you care to test?

>   imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>  
>   sport->dma_is_enabled = 0;
> -- 
> 1.8.3.1

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling

[Uwe Kleine-König]

Cc += Clemens Gruber + Fabio Estevam

On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> From: Nandor Han 
> 
> CTSC and CTS are not related to DMA and might add
> disruption in some cases.
> 
> Signed-off-by: Romain Perier 

If it was Nandor Han who created this patch, it would be great to get
his sob. If it was you, drop the From: line above.

> ---
>  drivers/tty/serial/imx.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5291b86..dd3ebb4 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
>   imx_stop_rx_dma(sport);
>   imx_stop_tx_dma(sport);
>  
> - /* clear UCR2 */
> - temp = readl(sport->port.membase + UCR2);
> - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> - writel(temp, sport->port.membase + UCR2);
> -

Before this patch imx_disable_dma resulted in the #CTS pin being high
(inactive).

Does this qualify as a fix? If so, you should sort this patch to the
beginning of the series. Did you do test this patch and its effects
separately?

@Clemens: maybe this patch makes a relevant difference when the port is
operated in rs485 mode. Do you care to test?

>   imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>  
>   sport->dma_is_enabled = 0;
> -- 
> 1.8.3.1

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |