Re: [PATCH] atmel serial reduce trasmitting window & code style patch

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Michael Trimarchi]

Hi,

> > the interrupt handler will be triggered as soon as we
> > enable the TX
> > interrupt. But perhaps we should avoid enabling the
> > interrupt and
> > schedule the tasklet since we _know_ there's a TX
> > interrupt pending?
> > 
> Ok, you are right, schedule the tasklet is better. I do
> some
> test and post another patch. 
> 
Just a comment. I can't schedule the tasklet for the tasklet itself
so maybe waiting for interrupt is a more clean solution.
What do you think?
> Michael
> 
> 
>       Inviato da Yahoo! Mail. 
> La casella di posta intelligente.
> http://it.docs.yahoo.com/mail/overview/index.html


      Inviato da Yahoo! Mail. 
La casella di posta intelligente.
http://it.docs.yahoo.com/mail/overview/index.html

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Michael Trimarchi]

Hi,


> For me this loop will never run more than 2-3 times.
> The CPU is much faster than the serial transmitter, so
> after 2 loops
> UART_GET_CSR(port) & ATMEL_US_TXRDY will be false and
> the loop will exit.
> 
Ok, I do a check, and count the max.

Michael



      Inviato da Yahoo! Mail. 
La casella di posta intelligente.
http://it.docs.yahoo.com/mail/overview/index.html

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Marc Pignat]

Hi all!

I just inlined your second patch for commenting.

On Thursday 03 April 2008, Michael Trimarchi wrote:
> Hi,
...
> http://it.docs.yahoo.com/mail/overview/index.html

> diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
> index 9f58eb3..511a0cc 100644
> --- a/drivers/serial/atmel_serial.c
> +++ b/drivers/serial/atmel_serial.c
> @@ -123,6 +123,7 @@ struct atmel_uart_char {
>  };
>  
>  #define ATMEL_SERIAL_RINGSIZE	1024
> +#define ATMEL_SERIAL_TXSIZE	32
>  
>  /*
>   * We wrap our port structure around the generic uart_port.
> @@ -426,6 +427,7 @@ static void atmel_rx_chars(struct uart_port *port)
>  static void atmel_tx_chars(struct uart_port *port)
>  {
>  	struct circ_buf *xmit = &port->info->xmit;
> +	int count = 0;
 
>  	if (port->x_char && UART_GET_CSR(port) & ATMEL_US_TXRDY) {
>  		UART_PUT_CHAR(port, port->x_char);
> @@ -435,10 +437,12 @@ static void atmel_tx_chars(struct uart_port *port)
>  	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
>  		return;
>  
> -	while (UART_GET_CSR(port) & ATMEL_US_TXRDY) {
> +	while (UART_GET_CSR(port) & ATMEL_US_TXRDY &&
> +	       count < ATMEL_SERIAL_TXSIZE) {
>  		UART_PUT_CHAR(port, xmit->buf[xmit->tail]);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		port->icount.tx++;
> +		count++;
>  		if (uart_circ_empty(xmit))
>  			break;
>  	}
For me this loop will never run more than 2-3 times.
The CPU is much faster than the serial transmitter, so after 2 loops
UART_GET_CSR(port) & ATMEL_US_TXRDY will be false and the loop will exit.

If your patch does something, I can't understand how!

Best regards

Marc

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Michael Trimarchi]

Hi,

> Ok, the patch seems to make sense, but your description
> isn't very good...
ok
>   * What problem does this patch solve (I think I know it,
> but I don't
>     want to guess.)
I try to resolve this problem:
The serial is set in interrupt mode 9600, and trasmit a lot of data.
The serial receive data from another device. The receive buffer is
1024 bytes and the trasmit buffer is an XMIT_SIZE. So when the tasklet
start to trasmit, it can loose data.
>   * This patch doesn't really reduce any "TX
> window" since the window
>     simply didn't exist before (or was infinitely long
> or whatever.) I
>     think you should mention that this limitation is a new
> thing.

>   * Why is 32 a good value?
> 
... I don't know a good value, because the tasklet can be preempted,
and the receiving buffer can be fill in background. It is hard to choose
a good value...
> Also, do we need to reschedule the tasklet if we terminate
> the loop
> because of this limit? I think we can get away with not
> doing it since
> the interrupt handler will be triggered as soon as we
> enable the TX
> interrupt. But perhaps we should avoid enabling the
> interrupt and
> schedule the tasklet since we _know_ there's a TX
> interrupt pending?
> 
Ok, you are right, schedule the tasklet is better. I do some
test and post another patch. 

Michael


      Inviato da Yahoo! Mail. 
La casella di posta intelligente.
http://it.docs.yahoo.com/mail/overview/index.html

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Haavard Skinnemoen]

On Thu, 3 Apr 2008 10:39:06 +0200 (CEST)
Michael Trimarchi <trimarchimichael@yahoo.it> wrote:

> Reduce the trasmitting window size to avoid blocking of tasklet because 
> it must handle the receive phase too.
> 
> Signed-off-by: michael <trimarchi@gandalf.sssup.it>

Ok, the patch seems to make sense, but your description isn't very good...
  * What problem does this patch solve (I think I know it, but I don't
    want to guess.)
  * This patch doesn't really reduce any "TX window" since the window
    simply didn't exist before (or was infinitely long or whatever.) I
    think you should mention that this limitation is a new thing.
  * Why is 32 a good value?

Also, do we need to reschedule the tasklet if we terminate the loop
because of this limit? I think we can get away with not doing it since
the interrupt handler will be triggered as soon as we enable the TX
interrupt. But perhaps we should avoid enabling the interrupt and
schedule the tasklet since we _know_ there's a TX interrupt pending?

Haavard

Re: [PATCH] atmel serial reduce trasmitting window & code style patch

[Haavard Skinnemoen]

On Thu, 3 Apr 2008 10:39:06 +0200 (CEST)
Michael Trimarchi <trimarchimichael@yahoo.it> wrote:

> Coding style clean-up patch.
> 
> Signed-off-by: michael <trimarchi@gandalf.sssup.it>

NAK. The existing style looks just fine to me.

If you want to line up the #defines, please just include that change in
the other patch. I'll comment on that in a few seconds.

Haavard

[PATCH] atmel serial reduce trasmitting window & code style patch

[Michael Trimarchi]

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

Hi,

two more patches for atmel serial driver.

Regards 
Michael



      Inviato da Yahoo! Mail. 
La casella di posta intelligente.
http://it.docs.yahoo.com/mail/overview/index.html

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 3607956876-coding-style-cleanup.patch --]
[-- Type: text/x-patch; name="coding-style-cleanup.patch", Size: 1749 bytes --]

Coding style clean-up patch.

Signed-off-by: michael <trimarchi@gandalf.sssup.it>

---
 drivers/serial/atmel_serial.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index d57bf3e..9f58eb3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -122,7 +122,7 @@ struct atmel_uart_char {
 	u16		ch;
 };
 
-#define ATMEL_SERIAL_RINGSIZE 1024
+#define ATMEL_SERIAL_RINGSIZE	1024
 
 /*
  * We wrap our port structure around the generic uart_port.
@@ -394,8 +394,8 @@ static void atmel_rx_chars(struct uart_port *port)
 			/* clear error */
 			UART_PUT_CR(port, ATMEL_US_RSTSTA);
 
-			if (status & ATMEL_US_RXBRK
-			    && !atmel_port->break_active) {
+			if (status & ATMEL_US_RXBRK &&
+			    !atmel_port->break_active) {
 				atmel_port->break_active = 1;
 				UART_PUT_IER(port, ATMEL_US_RXBRK);
 			} else {
@@ -1394,8 +1394,8 @@ console_initcall(atmel_console_init);
  */
 static int __init atmel_late_console_init(void)
 {
-	if (atmel_default_console_device
-	    && !(atmel_console.flags & CON_ENABLED))
+	if (atmel_default_console_device &&
+	    !(atmel_console.flags & CON_ENABLED))
 		register_console(&atmel_console);
 
 	return 0;
@@ -1434,8 +1434,8 @@ static int atmel_serial_suspend(struct platform_device *pdev,
 	struct uart_port *port = platform_get_drvdata(pdev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 
-	if (device_may_wakeup(&pdev->dev)
-	    && !at91_suspend_entering_slow_clock())
+	if (device_may_wakeup(&pdev->dev) &&
+	    !at91_suspend_entering_slow_clock())
 		enable_irq_wake(port->irq);
 	else {
 		uart_suspend_port(&atmel_uart, port);
-- 
1.5.2.1.174.gcd03-dirty


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 3362687731-atmel-interrupt-tx-chars-fix.patch --]
[-- Type: text/x-patch; name="atmel-interrupt-tx-chars-fix.patch", Size: 1405 bytes --]

Reduce the trasmitting window size to avoid blocking of tasklet because 
it must handle the receive phase too.

Signed-off-by: michael <trimarchi@gandalf.sssup.it>

---
 drivers/serial/atmel_serial.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 9f58eb3..511a0cc 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -123,6 +123,7 @@ struct atmel_uart_char {
 };
 
 #define ATMEL_SERIAL_RINGSIZE	1024
+#define ATMEL_SERIAL_TXSIZE	32
 
 /*
  * We wrap our port structure around the generic uart_port.
@@ -426,6 +427,7 @@ static void atmel_rx_chars(struct uart_port *port)
 static void atmel_tx_chars(struct uart_port *port)
 {
 	struct circ_buf *xmit = &port->info->xmit;
+	int count = 0;
 
 	if (port->x_char && UART_GET_CSR(port) & ATMEL_US_TXRDY) {
 		UART_PUT_CHAR(port, port->x_char);
@@ -435,10 +437,12 @@ static void atmel_tx_chars(struct uart_port *port)
 	if (uart_circ_empty(xmit) || uart_tx_stopped(port))
 		return;
 
-	while (UART_GET_CSR(port) & ATMEL_US_TXRDY) {
+	while (UART_GET_CSR(port) & ATMEL_US_TXRDY &&
+	       count < ATMEL_SERIAL_TXSIZE) {
 		UART_PUT_CHAR(port, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
+		count++;
 		if (uart_circ_empty(xmit))
 			break;
 	}
-- 
1.5.2.1.174.gcd03-dirty