[PATCH] CPM_UART: Fix non-console transmit

[PATCH] CPM_UART: Fix non-console transmit

[kalle.pokki]

The SMC and SCC hardware transmitter is enabled at the wrong
place. Simply writing twice to the non-console port, like

$ echo asdf > /dev/ttyCPM1
$ echo asdf > /dev/ttyCPM1

puts the shell into endless uninterruptible sleep, since the
transmitter is stopped after the first write, and is not enabled
before the shutdown function of the second write. Thus the transmit
buffers are never emptied.

Signed-off-by: Kalle Pokki <kalle.pokki@iki.fi>
---
  drivers/serial/cpm_uart/cpm_uart_core.c |    5 ++---
  1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index 90ff96e..8f3b3e5 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -194,10 +194,8 @@ static void cpm_uart_start_tx(struct uar
  	if (cpm_uart_tx_pump(port) != 0) {
  		if (IS_SMC(pinfo)) {
  			smcp->smc_smcm |= SMCM_TX;
-			smcp->smc_smcmr |= SMCMR_TEN;
  		} else {
  			sccp->scc_sccm |= UART_SCCM_TX;
-			pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENT;
  		}
  	}
  }
@@ -420,9 +418,10 @@ static int cpm_uart_startup(struct uart_
  	/* Startup rx-int */
  	if (IS_SMC(pinfo)) {
  		pinfo->smcp->smc_smcm |= SMCM_RX;
-		pinfo->smcp->smc_smcmr |= SMCMR_REN;
+		pinfo->smcp->smc_smcmr |= (SMCMR_REN | SMCMR_TEN);
  	} else {
  		pinfo->sccp->scc_sccm |= UART_SCCM_RX;
+		pinfo->sccp->scc_gsmrl |= (SCC_GSMRL_ENR | SCC_GSMRL_ENT);
  	}

  	if (!(pinfo->flags & FLAG_CONSOLE))
-- 
1.4.1.1

[PATCH] CPM_UART: Fix non-console transmit

[Kalle Pokki]

The SMC and SCC hardware transmitter is enabled at the wrong
place. Simply writing twice to the non-console port, like

$ echo asdf > /dev/ttyCPM1
$ echo asdf > /dev/ttyCPM1

puts the shell into endless uninterruptible sleep, since the
transmitter is stopped after the first write, and is not enabled
before the shutdown function of the second write. Thus the transmit
buffers are never emptied.

Signed-off-by: Kalle Pokki <kalle.pokki@iki.fi>
---
  drivers/serial/cpm_uart/cpm_uart_core.c |    5 ++---
  1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index 90ff96e..8f3b3e5 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -194,10 +194,8 @@ static void cpm_uart_start_tx(struct uar
  	if (cpm_uart_tx_pump(port) != 0) {
  		if (IS_SMC(pinfo)) {
  			smcp->smc_smcm |= SMCM_TX;
-			smcp->smc_smcmr |= SMCMR_TEN;
  		} else {
  			sccp->scc_sccm |= UART_SCCM_TX;
-			pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENT;
  		}
  	}
  }
@@ -420,9 +418,10 @@ static int cpm_uart_startup(struct uart_
  	/* Startup rx-int */
  	if (IS_SMC(pinfo)) {
  		pinfo->smcp->smc_smcm |= SMCM_RX;
-		pinfo->smcp->smc_smcmr |= SMCMR_REN;
+		pinfo->smcp->smc_smcmr |= (SMCMR_REN | SMCMR_TEN);
  	} else {
  		pinfo->sccp->scc_sccm |= UART_SCCM_RX;
+		pinfo->sccp->scc_gsmrl |= (SCC_GSMRL_ENR | SCC_GSMRL_ENT);
  	}

  	if (!(pinfo->flags & FLAG_CONSOLE))
-- 
1.4.1.1

Re: [PATCH] CPM_UART: Fix non-console transmit

[Boris Shteinbock]

Kalle.

After applying your patch,
the transmission doesn't take place at all..
just hanging in write...
at least it is possible to exit the program.

Re: [PATCH] CPM_UART: Fix non-console transmit

[Kalle Pokki]

On 10/19/06, Boris Shteinbock <boris@fabiotec.com> wrote:
>
> After applying your patch,
> the transmission doesn't take place at all..
> just hanging in write...
> at least it is possible to exit the program.

That's strange. No matter how I test the driver it now works
perfectly for me with my patches. Did you apply them both? However, I
cannot see why the length of the transmission would cause any
difference.

What is you baud rate on SMC2? How about the TX buffer count and TX
FIFO size - have you changed them? I suspect that cpm_uart_tx_pump()
could disable your TX interrupt when you have all characters queued in
separate TX buffers. I'll test this theory tomorrow.

You might want to strace your program and post the results here. Also put

  #define DEBUG

in front of cpm_uart_core.c and cpm_uart_cpm2.c and recompile. You'll
get a lot of debugging messages after

  echo 8 > /proc/sys/kernel/printk

That should give us clues what goes wrong. The output of

  stty -F /dev/ttyCPM1 -a

would also help.

Re: [PATCH] CPM_UART: Fix non-console transmit

[Vitaly Bordug]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

On Thu, 19 Oct 2006 15:25:55 +0300 (EEST)
kalle.pokki@iki.fi wrote:

> The SMC and SCC hardware transmitter is enabled at the wrong
> place. Simply writing twice to the non-console port, like
> 
> $ echo asdf > /dev/ttyCPM1
> $ echo asdf > /dev/ttyCPM1
> 
> puts the shell into endless uninterruptible sleep, since the
> transmitter is stopped after the first write, and is not enabled
> before the shutdown function of the second write. Thus the transmit
> buffers are never emptied.
> 
Was about to Ack, but... need one more glance at it on hw. 
I recall something that might be reason of the current implementation,
but had no chance to recover the details.

Thanks for the input. I'll follow-up tomorrow.

> Signed-off-by: Kalle Pokki <kalle.pokki@iki.fi>
> ---
>   drivers/serial/cpm_uart/cpm_uart_core.c |    5 ++---
>   1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c
> b/drivers/serial/cpm_uart/cpm_uart_core.c index 90ff96e..8f3b3e5
> 100644 --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -194,10 +194,8 @@ static void cpm_uart_start_tx(struct uar
>   	if (cpm_uart_tx_pump(port) != 0) {
>   		if (IS_SMC(pinfo)) {
>   			smcp->smc_smcm |= SMCM_TX;
> -			smcp->smc_smcmr |= SMCMR_TEN;
>   		} else {
>   			sccp->scc_sccm |= UART_SCCM_TX;
> -			pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENT;
>   		}
>   	}
>   }
> @@ -420,9 +418,10 @@ static int cpm_uart_startup(struct uart_
>   	/* Startup rx-int */
>   	if (IS_SMC(pinfo)) {
>   		pinfo->smcp->smc_smcm |= SMCM_RX;
> -		pinfo->smcp->smc_smcmr |= SMCMR_REN;
> +		pinfo->smcp->smc_smcmr |= (SMCMR_REN | SMCMR_TEN);
>   	} else {
>   		pinfo->sccp->scc_sccm |= UART_SCCM_RX;
> +		pinfo->sccp->scc_gsmrl |= (SCC_GSMRL_ENR |
> SCC_GSMRL_ENT); }
> 
>   	if (!(pinfo->flags & FLAG_CONSOLE))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] CPM_UART: Fix non-console transmit

[Laurent Pinchart]

> > After applying your patch,
> > the transmission doesn't take place at all..
> > just hanging in write...
> > at least it is possible to exit the program.
>
> That's strange. No matter how I test the driver it now works
> perfectly for me with my patches.

Ditto. Your patch fixed my problems. I'd like to see a fix (either this one or 
another one, there are different solutions) pushed upstream soon, as the 
problem is quite serious. I'm quite amazed nobody had ever reported it 
before.

> Did you apply them both?

I only applied [PATCH] CPM_UART: Fix non-console transmit

> However, I cannot see why the length of the transmission would cause any
> difference.
>
> What is you baud rate on SMC2? How about the TX buffer count and TX
> FIFO size - have you changed them? I suspect that cpm_uart_tx_pump()
> could disable your TX interrupt when you have all characters queued in
> separate TX buffers. I'll test this theory tomorrow.
>
> You might want to strace your program and post the results here. Also put
>
>   #define DEBUG
>
> in front of cpm_uart_core.c and cpm_uart_cpm2.c and recompile. You'll
> get a lot of debugging messages after
>
>   echo 8 > /proc/sys/kernel/printk
>
> That should give us clues what goes wrong. The output of
>
>   stty -F /dev/ttyCPM1 -a
>
> would also help.

Cheers,

Laurent Pinchart

Re: [PATCH] CPM_UART: Fix non-console transmit

[Kalle Pokki]

On 10/31/06, Laurent Pinchart <laurent.pinchart@tbox.biz> wrote:
> Ditto. Your patch fixed my problems. I'd like to see a fix (either this one or
> another one, there are different solutions) pushed upstream soon, as the
> problem is quite serious. I'm quite amazed nobody had ever reported it
> before.

I think cpm_uart_startup() is the right place to enable both
transmitter and receiver, since they are disabled in the corresponding
shutdown() function. Or actually, one might be just fine with never
disabling the hardware at all. I can't see any reason other than power
consumption that they are disabled by the shutdown() function.

It seems the reason of not enabling the transmitter at startup() is
that the serial core specification only states that reception should
be enabled with that function call. However enabling also the
transmitter does not by itself initiate any transmissions, so it
should be just fine. It just makes the hardware ready to transmit.

Re: [PATCH] CPM_UART: Fix non-console transmit

[Vitaly Bordug]

On Thu, 19 Oct 2006 15:52:11 +0300 (EEST)
Kalle Pokki <kalle.pokki@iki.fi> wrote:

> The SMC and SCC hardware transmitter is enabled at the wrong
> place. Simply writing twice to the non-console port, like
> 
> $ echo asdf > /dev/ttyCPM1
> $ echo asdf > /dev/ttyCPM1
> 
> puts the shell into endless uninterruptible sleep, since the
> transmitter is stopped after the first write, and is not enabled
> before the shutdown function of the second write. Thus the transmit
> buffers are never emptied.
> 
> Signed-off-by: Kalle Pokki <kalle.pokki@iki.fi>
Signed-off-by: Vitaly Bordug <vbordug@ru.mvista.com>

Paul: please apply. 

I am recalling addressing something like the upper, but dunno why it haven't pushed 
upstream. Something around re-use scc/smc did not feel good with this approach, but it is 
much less important than the odd behavior expressed.

-- 
Sincerely, 
Vitaly

Re: [PATCH] CPM_UART: Fix non-console transmit

[Paul Mackerras]

Vitaly Bordug writes:

> Paul: please apply. 
> 
> I am recalling addressing something like the upper, but dunno why it haven't pushed 
> upstream. Something around re-use scc/smc did not feel good with this approach, but it is 
> much less important than the odd behavior expressed.

I don't have this patch, could someone send it to me please (and if
you want it in 2.6.19, make sure the commentary explains what bug it
is fixing).

Paul.

Re: [PATCH] CPM_UART: Fix non-console transmit

[Kalle Pokki]

On Wed, 1 Nov 2006, Paul Mackerras wrote:

> Vitaly Bordug writes:
> 
> > Paul: please apply. 
> > 
> > I am recalling addressing something like the upper, but dunno why it haven't pushed 
> > upstream. Something around re-use scc/smc did not feel good with this approach, but it is 
> > much less important than the odd behavior expressed.
> 
> I don't have this patch, could someone send it to me please (and if
> you want it in 2.6.19, make sure the commentary explains what bug it
> is fixing).

Here it goes again.

Kalle
---
CPM_UART: Fix non-console transmit

The SMC and SCC hardware transmitter is enabled at the wrong
place. Simply writing twice to the non-console port, like

$ echo asdf > /dev/ttyCPM1
$ echo asdf > /dev/ttyCPM1

puts the shell into endless uninterruptible sleep, since the
transmitter is stopped after the first write, and is not enabled
before the shutdown function of the second write. Thus the transmit
buffers are never emptied.

Signed-off-by: Kalle Pokki <kalle.pokki@iki.fi>
Signed-off-by: Vitaly Bordug <vbordug@ru.mvista.com>
---
  drivers/serial/cpm_uart/cpm_uart_core.c |    5 ++---
  1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index 90ff96e..8f3b3e5 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -194,10 +194,8 @@ static void cpm_uart_start_tx(struct uar
  	if (cpm_uart_tx_pump(port) != 0) {
  		if (IS_SMC(pinfo)) {
  			smcp->smc_smcm |= SMCM_TX;
-			smcp->smc_smcmr |= SMCMR_TEN;
  		} else {
  			sccp->scc_sccm |= UART_SCCM_TX;
-			pinfo->sccp->scc_gsmrl |= SCC_GSMRL_ENT;
  		}
  	}
  }
@@ -420,9 +418,10 @@ static int cpm_uart_startup(struct uart_
  	/* Startup rx-int */
  	if (IS_SMC(pinfo)) {
  		pinfo->smcp->smc_smcm |= SMCM_RX;
-		pinfo->smcp->smc_smcmr |= SMCMR_REN;
+		pinfo->smcp->smc_smcmr |= (SMCMR_REN | SMCMR_TEN);
  	} else {
  		pinfo->sccp->scc_sccm |= UART_SCCM_RX;
+		pinfo->sccp->scc_gsmrl |= (SCC_GSMRL_ENR | SCC_GSMRL_ENT);
  	}

  	if (!(pinfo->flags & FLAG_CONSOLE))
-- 
1.4.1.1