linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] atmel_serial: update the powersave handler to match serial core
@ 2008-09-19 15:11 Haavard Skinnemoen
  2008-09-19 15:19 ` Anti Sullin
  0 siblings, 1 reply; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-19 15:11 UTC (permalink / raw)
  To: linux-serial; +Cc: linux-kernel, Andrew Victor, Anti Sullin, Haavard Skinnemoen

From: Anti Sullin <anti.sullin@artecdesign.ee>

This problem seems to be unnoticed so far:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b3b708fa2780cd2b5d8266a8f0c3a1cab364d4d2

has changed the serial core behavior to not to suspend the port if the
device is enabled as a wakeup source. If the AT91 system goes to slow
clock mode, the port should be suspended always and the clocks should be
switched off.  The patch attached updates the atmel_serial driver to
match the changes in serial core.

Also, the interrupts are disabled when the clock is disabled. If we
disable the clock with interrupts enabled, an interrupt may get stuck.
If this is the DBGU interrupt, this blocks the OR logic at system
controller and thus all other sysc interrupts.

Signed-off-by: Anti Sullin <anti.sullin@artecdesign.ee>
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 drivers/serial/atmel_serial.c |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 3a6da80..eeb2ca3 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -131,7 +131,8 @@ struct atmel_uart_char {
 struct atmel_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
-	unsigned short		suspended;	/* is port suspended? */
+	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
+	u32			backup_imr;	/* IMR saved during suspend */
 	int			break_active;	/* break being received */
 
 	short			use_dma_rx;	/* enable PDC receiver */
@@ -984,8 +985,15 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 		 * This is called on uart_open() or a resume event.
 		 */
 		clk_enable(atmel_port->clk);
+
+		/* re-enable interrupts if we disabled some on suspend */
+		UART_PUT_IER(port, atmel_port->backup_imr);
 		break;
 	case 3:
+		/* Back up the interrupt mask and disable all interrupts */
+		atmel_port->backup_imr = UART_GET_IMR(port);
+		UART_PUT_IDR(port, -1);
+
 		/*
 		 * Disable the peripheral clock for this serial port.
 		 * This is called on uart_close() or a suspend event.
@@ -1475,13 +1483,12 @@ static int atmel_serial_suspend(struct platform_device *pdev,
 			cpu_relax();
 	}
 
-	if (device_may_wakeup(&pdev->dev)
-	    && !atmel_serial_clk_will_stop())
-		enable_irq_wake(port->irq);
-	else {
-		uart_suspend_port(&atmel_uart, port);
-		atmel_port->suspended = 1;
-	}
+	/* we can not wake up if we're running on slow clock */
+	atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
+	if (atmel_serial_clk_will_stop())
+		device_set_wakeup_enable(&pdev->dev, 0);
+
+	uart_suspend_port(&atmel_uart, port);
 
 	return 0;
 }
@@ -1491,11 +1498,9 @@ static int atmel_serial_resume(struct platform_device *pdev)
 	struct uart_port *port = platform_get_drvdata(pdev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 
-	if (atmel_port->suspended) {
-		uart_resume_port(&atmel_uart, port);
-		atmel_port->suspended = 0;
-	} else
-		disable_irq_wake(port->irq);
+	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
+
+	uart_resume_port(&atmel_uart, port);
 
 	return 0;
 }
@@ -1513,6 +1518,8 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
 	BUILD_BUG_ON(!is_power_of_2(ATMEL_SERIAL_RINGSIZE));
 
 	port = &atmel_ports[pdev->id];
+	port->backup_imr = 0;
+
 	atmel_init_port(port, pdev);
 
 	if (!atmel_use_dma_rx(&port->uart)) {
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 15:11 [PATCH] atmel_serial: update the powersave handler to match serial core Haavard Skinnemoen
@ 2008-09-19 15:19 ` Anti Sullin
  2008-09-19 15:39   ` [PATCH v2] " Haavard Skinnemoen
  0 siblings, 1 reply; 12+ messages in thread
From: Anti Sullin @ 2008-09-19 15:19 UTC (permalink / raw)
  To: Haavard Skinnemoen; +Cc: linux-serial, linux-kernel, Andrew Victor

One more bug of mine...

Haavard Skinnemoen wrote:
>  }
> @@ -1491,11 +1498,9 @@ static int atmel_serial_resume(struct platform_device *pdev)
>  	struct uart_port *port = platform_get_drvdata(pdev);
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  
> -	if (atmel_port->suspended) {
> -		uart_resume_port(&atmel_uart, port);
> -		atmel_port->suspended = 0;
> -	} else
> -		disable_irq_wake(port->irq);
> +	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
> +
> +	uart_resume_port(&atmel_uart, port);
Just now noticed - these two lines should be switched.
>  
>  	return 0;
>  }



-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Akadeemia tee 23A, 12618, Tallinn, Estonia
http://www.artecdesign.ee 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
@ 2008-09-19 15:33 Michael Trimarchi
  2008-09-19 15:46 ` Haavard Skinnemoen
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Trimarchi @ 2008-09-19 15:33 UTC (permalink / raw)
  To: linux-serial; +Cc: linux-kernel, Andrew Victor, Anti Sullin, Haavard Skinnemoen

Hi,




> +    /* we can not wake up if we're running on slow clock */
> +    atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
> +    if (atmel_serial_clk_will_stop())
> +        device_set_wakeup_enable(&pdev->dev, 0);
> +
This is not true, the wakeup in the slow clock is possible, configure the
input pin of the serial device as a gpio, and wake up on gpio. Then you
must reconfigure as a serial pin, and you can wake up on slow clock.

Regards Michael

__________________________________________________
Do You Yahoo!?
Poco spazio e tanto spam? Yahoo! Mail ti protegge dallo spam e ti da tanto spazio gratuito per i tuoi file e i messaggi 
http://mail.yahoo.it 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] atmel_serial: update the powersave handler to match serial core
  2008-09-19 15:19 ` Anti Sullin
@ 2008-09-19 15:39   ` Haavard Skinnemoen
  0 siblings, 0 replies; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-19 15:39 UTC (permalink / raw)
  To: linux-serial; +Cc: linux-kernel, Andrew Victor, Anti Sullin, Haavard Skinnemoen

From: Anti Sullin <anti.sullin@artecdesign.ee>

This problem seems to be unnoticed so far:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b3b708fa2780cd2b5d8266a8f0c3a1cab364d4d2

has changed the serial core behavior to not to suspend the port if the
device is enabled as a wakeup source. If the AT91 system goes to slow
clock mode, the port should be suspended always and the clocks should be
switched off.  The patch attached updates the atmel_serial driver to
match the changes in serial core.

Also, the interrupts are disabled when the clock is disabled. If we
disable the clock with interrupts enabled, an interrupt may get stuck.
If this is the DBGU interrupt, this blocks the OR logic at system
controller and thus all other sysc interrupts.

Signed-off-by: Anti Sullin <anti.sullin@artecdesign.ee>
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
Anti Sullin <anti.sullin@artecdesign.ee> wrote:
> > +	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
> > +
> > +	uart_resume_port(&atmel_uart, port);
> Just now noticed - these two lines should be switched.

Makes sense to me.

 drivers/serial/atmel_serial.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 3a6da80..61fb8b6 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -131,7 +131,8 @@ struct atmel_uart_char {
 struct atmel_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
-	unsigned short		suspended;	/* is port suspended? */
+	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
+	u32			backup_imr;	/* IMR saved during suspend */
 	int			break_active;	/* break being received */
 
 	short			use_dma_rx;	/* enable PDC receiver */
@@ -984,8 +985,15 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 		 * This is called on uart_open() or a resume event.
 		 */
 		clk_enable(atmel_port->clk);
+
+		/* re-enable interrupts if we disabled some on suspend */
+		UART_PUT_IER(port, atmel_port->backup_imr);
 		break;
 	case 3:
+		/* Back up the interrupt mask and disable all interrupts */
+		atmel_port->backup_imr = UART_GET_IMR(port);
+		UART_PUT_IDR(port, -1);
+
 		/*
 		 * Disable the peripheral clock for this serial port.
 		 * This is called on uart_close() or a suspend event.
@@ -1475,13 +1483,12 @@ static int atmel_serial_suspend(struct platform_device *pdev,
 			cpu_relax();
 	}
 
-	if (device_may_wakeup(&pdev->dev)
-	    && !atmel_serial_clk_will_stop())
-		enable_irq_wake(port->irq);
-	else {
-		uart_suspend_port(&atmel_uart, port);
-		atmel_port->suspended = 1;
-	}
+	/* we can not wake up if we're running on slow clock */
+	atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
+	if (atmel_serial_clk_will_stop())
+		device_set_wakeup_enable(&pdev->dev, 0);
+
+	uart_suspend_port(&atmel_uart, port);
 
 	return 0;
 }
@@ -1491,11 +1498,8 @@ static int atmel_serial_resume(struct platform_device *pdev)
 	struct uart_port *port = platform_get_drvdata(pdev);
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 
-	if (atmel_port->suspended) {
-		uart_resume_port(&atmel_uart, port);
-		atmel_port->suspended = 0;
-	} else
-		disable_irq_wake(port->irq);
+	uart_resume_port(&atmel_uart, port);
+	device_set_wakeup_enable(&pdev->dev, atmel_port->may_wakeup);
 
 	return 0;
 }
@@ -1513,6 +1517,8 @@ static int __devinit atmel_serial_probe(struct platform_device *pdev)
 	BUILD_BUG_ON(!is_power_of_2(ATMEL_SERIAL_RINGSIZE));
 
 	port = &atmel_ports[pdev->id];
+	port->backup_imr = 0;
+
 	atmel_init_port(port, pdev);
 
 	if (!atmel_use_dma_rx(&port->uart)) {
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 15:33 [PATCH] " Michael Trimarchi
@ 2008-09-19 15:46 ` Haavard Skinnemoen
  0 siblings, 0 replies; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-19 15:46 UTC (permalink / raw)
  To: Michael Trimarchi; +Cc: linux-serial, linux-kernel, Andrew Victor, Anti Sullin

Michael Trimarchi <trimarchimichael@yahoo.it> wrote:
> > +    /* we can not wake up if we're running on slow clock */
> > +    atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
> > +    if (atmel_serial_clk_will_stop())
> > +        device_set_wakeup_enable(&pdev->dev, 0);
> > +  
> This is not true, the wakeup in the slow clock is possible, configure the
> input pin of the serial device as a gpio, and wake up on gpio. Then you
> must reconfigure as a serial pin, and you can wake up on slow clock.

Yes, but the current driver doesn't actually support that, does it?

This patch doesn't really change that assumption anyway -- it just
implements it in a way more consistent with the current serial core.

Haavard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
@ 2008-09-19 16:01 Michael Trimarchi
  2008-09-19 16:15 ` Haavard Skinnemoen
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Trimarchi @ 2008-09-19 16:01 UTC (permalink / raw)
  To: Haavard Skinnemoen; +Cc: linux-serial, linux-kernel, Andrew Victor, Anti Sullin

Hi,


(I'll change my mailer soon ...), sorry if I break the thread.


----- Messaggio originale -----
> Da: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> A: Michael Trimarchi <trimarchimichael@yahoo.it>
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Victor <linux@maxim.org.za>; Anti Sullin <anti.sullin@artecdesign.ee>
> Inviato: Venerdì 19 settembre 2008, 17:46:15
> Oggetto: Re: [PATCH] atmel_serial: update the powersave handler to match serial core
> 
> Michael Trimarchi wrote:
> > > +    /* we can not wake up if we're running on slow clock */
> > > +    atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
> > > +    if (atmel_serial_clk_will_stop())
> > > +        device_set_wakeup_enable(&pdev->dev, 0);
> > > +  
> > This is not true, the wakeup in the slow clock is possible, configure the
> > input pin of the serial device as a gpio, and wake up on gpio. Then you
> > must reconfigure as a serial pin, and you can wake up on slow clock.
> 
> Yes, but the current driver doesn't actually support that, does it?
> 

Yes I know, but we can think about it and support when is possible
to wake-up the serial device. Each serial device can change the status
of the gpio-pin before suspendig and register and handler on it (I think one
interrupt handler for all the serial that can do that), then reset it after resume.
I think not all the serial device can do that in the atmel architectures but I suppose
that can be very usefull for example in gprs application.

> This patch doesn't really change that assumption anyway -- it just
> implements it in a way more consistent with the current serial core.
> 

> Haavard

Regards Michael

__________________________________________________
Do You Yahoo!?
Poco spazio e tanto spam? Yahoo! Mail ti protegge dallo spam e ti da tanto spazio gratuito per i tuoi file e i messaggi 
http://mail.yahoo.it 
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 16:01 Michael Trimarchi
@ 2008-09-19 16:15 ` Haavard Skinnemoen
  0 siblings, 0 replies; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-19 16:15 UTC (permalink / raw)
  To: Michael Trimarchi; +Cc: linux-serial, linux-kernel, Andrew Victor, Anti Sullin

Michael Trimarchi <trimarchimichael@yahoo.it> wrote:
> > Michael Trimarchi wrote:  
> > > > +    /* we can not wake up if we're running on slow clock */
> > > > +    atmel_port->may_wakeup = device_may_wakeup(&pdev->dev);
> > > > +    if (atmel_serial_clk_will_stop())
> > > > +        device_set_wakeup_enable(&pdev->dev, 0);
> > > > +    
> > > This is not true, the wakeup in the slow clock is possible, configure the
> > > input pin of the serial device as a gpio, and wake up on gpio. Then you
> > > must reconfigure as a serial pin, and you can wake up on slow clock.  
> > 
> > Yes, but the current driver doesn't actually support that, does it?
> >   
> 
> Yes I know, but we can think about it and support when is possible
> to wake-up the serial device. Each serial device can change the status
> of the gpio-pin before suspendig and register and handler on it (I think one
> interrupt handler for all the serial that can do that), then reset it after resume.
> I think not all the serial device can do that in the atmel architectures but I suppose
> that can be very usefull for example in gprs application.

I agree it would be useful. It would require changing the port mux
configuration from the driver though, and there's no standardized
interface for doing that. Maybe this is a good motivation to come up
with one?

Btw, I assume the first character you receive will be lost when you do
this, right?

Haavard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
@ 2008-09-19 16:33 Michael Trimarchi
  2008-09-19 18:35 ` Haavard Skinnemoen
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Trimarchi @ 2008-09-19 16:33 UTC (permalink / raw)
  To: Haavard Skinnemoen; +Cc: linux-serial, linux-kernel, Andrew Victor, Anti Sullin

Hi,



----- Messaggio originale -----
> Da: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> A: Michael Trimarchi <trimarchimichael@yahoo.it>
> Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Victor <linux@maxim.org.za>; Anti Sullin <anti.sullin@artecdesign.ee>
> Inviato: Venerdì 19 settembre 2008, 18:15:51
> Oggetto: Re: [PATCH] atmel_serial: update the powersave handler to match serial core
> 

> I agree it would be useful. It would require changing the port mux
> configuration from the driver though, and there's no standardized
> interface for doing that. Maybe this is a good motivation to come up
> with one?
> 

I think that a driver can do the request to a the gpio layer (may can be implemented
by the gpio-lib ) and give it only the gpio. The "gpio-lib" can save and restore
the status of the gpio, and request the handler, passing the gpio-id as 
a data. So when the handler fire, we can now which peripheral is
interested on the wake-up event.

> Btw, I assume the first character you receive will be lost when you do
> this, right?
>

Yes, I haven't done a lot of test to see how many chars are lost (sure one I think). 
Depends on the time spent after

 /* Wait for interrupt to wake us up */ 
  mcr p15, 0, r0, c7, c0, 4 
 
> Haavard

Regards Michael


__________________________________________________
Do You Yahoo!?
Poco spazio e tanto spam? Yahoo! Mail ti protegge dallo spam e ti da tanto spazio gratuito per i tuoi file e i messaggi 
http://mail.yahoo.it 
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 16:33 Michael Trimarchi
@ 2008-09-19 18:35 ` Haavard Skinnemoen
  2008-09-19 21:49   ` Anti Sullin
  0 siblings, 1 reply; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-19 18:35 UTC (permalink / raw)
  To: Michael Trimarchi; +Cc: linux-serial, linux-kernel, Andrew Victor, Anti Sullin

Michael Trimarchi <trimarchimichael@yahoo.it> wrote:
> > I agree it would be useful. It would require changing the port mux
> > configuration from the driver though, and there's no standardized
> > interface for doing that. Maybe this is a good motivation to come up
> > with one?
> >   
> 
> I think that a driver can do the request to a the gpio layer (may can be implemented
> by the gpio-lib ) and give it only the gpio. The "gpio-lib" can save and restore
> the status of the gpio, and request the handler, passing the gpio-id as 
> a data. So when the handler fire, we can now which peripheral is
> interested on the wake-up event.

I don't think the gpio layer is supposed to touch the portmux. David
has always been very clear about that. But if we somehow manage to get
the pin configured as a GPIO, we can use the GPIO layer to request a
pin change interrupt.

It _might_ work even if you don't reconfigure the pin as a GPIO...but
then I think we'd be relying on undocumented behaviour.

> > Btw, I assume the first character you receive will be lost when you do
> > this, right?
> >  
> 
> Yes, I haven't done a lot of test to see how many chars are lost (sure one I think). 
> Depends on the time spent after
> 
>  /* Wait for interrupt to wake us up */ 
>   mcr p15, 0, r0, c7, c0, 4 

Yes, we need to reach the atmel_serial resume handler before the UART
gets any chance to recognize the character. It's probably too late for
the first one, but it may catch the next one assuming the baud rate
isn't too high.

Haavard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 18:35 ` Haavard Skinnemoen
@ 2008-09-19 21:49   ` Anti Sullin
  2008-09-20 13:01     ` Haavard Skinnemoen
  0 siblings, 1 reply; 12+ messages in thread
From: Anti Sullin @ 2008-09-19 21:49 UTC (permalink / raw)
  To: Haavard Skinnemoen
  Cc: Michael Trimarchi, linux-serial, linux-kernel, Andrew Victor

Haavard Skinnemoen wrote:
> Michael Trimarchi <trimarchimichael@yahoo.it> wrote:
>   
>>> I agree it would be useful. It would require changing the port mux
>>> configuration from the driver though, and there's no standardized
>>> interface for doing that. Maybe this is a good motivation to come up
>>> with one?
>>>   
>>>       
>> I think that a driver can do the request to a the gpio layer (may can be implemented
>> by the gpio-lib ) and give it only the gpio. The "gpio-lib" can save and restore
>> the status of the gpio, and request the handler, passing the gpio-id as 
>> a data. So when the handler fire, we can now which peripheral is
>> interested on the wake-up event.
>>     
>
> I don't think the gpio layer is supposed to touch the portmux. David
> has always been very clear about that. But if we somehow manage to get
> the pin configured as a GPIO, we can use the GPIO layer to request a
> pin change interrupt.
>
> It _might_ work even if you don't reconfigure the pin as a GPIO...but
> then I think we'd be relying on undocumented behaviour.
>   
I believe that you don't need to reconfigure the pin. The gpio hw does
not require the pin to be multiplexed to any given state, so does not request_irq (correct?).
I did this trick in my [2/3] MCI driver patch I sent to arm-linux-kernel at 18.03.2008 (the
e-mail subject line was wrong, [1/3], though) and I'm using that patch still on my
production devices to avoid some rare but critical SD data corruption (mainline kernel still 
screws up the filesystem on SD sometimes). The same should be easily usable on serial port, too.

>   
>>> Btw, I assume the first character you receive will be lost when you do
>>> this, right?
>>>  
>>>       
>> Yes, I haven't done a lot of test to see how many chars are lost (sure one I think). 
>> Depends on the time spent after
>>
>>  /* Wait for interrupt to wake us up */ 
>>   mcr p15, 0, r0, c7, c0, 4 
>>     
>
> Yes, we need to reach the atmel_serial resume handler before the UART
> gets any chance to recognize the character. It's probably too late for
> the first one, but it may catch the next one assuming the baud rate
> isn't too high.
>   
I believe, that we loose quite many characters. We are running at 32/16kHz
(switching the master clock divider /2 off proved unstable) and
this is not enough to even receive 9600 baud until we have the fast clock
running again.

Some quick calculations:
For waking up, we need to switch on the main oscillator, wait until it
stabilizes (default should be max: ~60ms), switch on plla, wait until it
locks (2ms), switch on pllb, wait until it locks (2ms), switch system to PLLA...
And then set up the drivers again... On 115200 baud, this is almost 1kB!

So in many applications we could not use this. But this might still come handy
in a lot of cases we can poll and find out what caused the data on the serial
port etc. Or on applications, where this loss of data does not matter (like debug
console where the resume is usable even if it does not wake up on the first byte).


-- 
Anti Sullin
Embedded Software Engineer
Artec Design LLC
Akadeemia tee 23A, 12618, Tallinn, Estonia
http://www.artecdesign.ee 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
@ 2008-09-20  9:31 Michael Trimarchi
  0 siblings, 0 replies; 12+ messages in thread
From: Michael Trimarchi @ 2008-09-20  9:31 UTC (permalink / raw)
  To: Anti Sullin, Haavard Skinnemoen; +Cc: linux-serial, linux-kernel, Andrew Victor

Hi,

----- Messaggio originale -----
> Da: Anti Sullin <anti.sullin@artecdesign.ee>
> A: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> Cc: Michael Trimarchi <trimarchimichael@yahoo.it>; linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; Andrew Victor <linux@maxim.org.za>
> Inviato: Venerdì 19 settembre 2008, 23:49:29
> Oggetto: Re: [PATCH] atmel_serial: update the powersave handler to match serial core
> 
> Haavard Skinnemoen wrote:
> > Michael Trimarchi wrote:
> >  
> >>> I agree it would be useful. It would require changing the port mux
> >>> configuration from the driver though, and there's no standardized
> >>> interface for doing that. Maybe this is a good motivation to come up
> >>> with one?
> >>>  
> >>>      
> >> I think that a driver can do the request to a the gpio layer (may can be 
> implemented
> >> by the gpio-lib ) and give it only the gpio. The "gpio-lib" can save and 
> restore
> >> the status of the gpio, and request the handler, passing the gpio-id as 
> >> a data. So when the handler fire, we can now which peripheral is
> >> interested on the wake-up event.
> >>    
> >
> > I don't think the gpio layer is supposed to touch the portmux. David
> > has always been very clear about that. But if we somehow manage to get
> > the pin configured as a GPIO, we can use the GPIO layer to request a
> > pin change interrupt.
> >
> > It _might_ work even if you don't reconfigure the pin as a GPIO...but
> > then I think we'd be relying on undocumented behaviour.
> >  
> I believe that you don't need to reconfigure the pin. The gpio hw does
> not require the pin to be multiplexed to any given state, so does not 
> request_irq (correct?).
> I did this trick in my [2/3] MCI driver patch I sent to arm-linux-kernel at 
> 18.03.2008 (the
> e-mail subject line was wrong, [1/3], though) and I'm using that patch still on 
> my
> production devices to avoid some rare but critical SD data corruption (mainline 
> kernel still 
> screws up the filesystem on SD sometimes). The same should be easily usable on 
> serial port, too.
>

I must to check this point. I do it few months ago, and if I remember I change
the serial configuration on rx line to a gpio and request and interrupt. I don't
have the hardware to recheck.

> >  
> >>> Btw, I assume the first character you receive will be lost when you do
> >>> this, right?
> >>>  
> >>>      
> >> Yes, I haven't done a lot of test to see how many chars are lost (sure one I 
> think). 
> >> Depends on the time spent after
> >>
> >>  /* Wait for interrupt to wake us up */ 
> >>   mcr p15, 0, r0, c7, c0, 4 
> >>    
> >
> > Yes, we need to reach the atmel_serial resume handler before the UART
> > gets any chance to recognize the character. It's probably too late for
> > the first one, but it may catch the next one assuming the baud rate
> > isn't too high.
> >  
> I believe, that we loose quite many characters. We are running at 32/16kHz
> (switching the master clock divider /2 off proved unstable) and
> this is not enough to even receive 9600 baud until we have the fast clock
> running again.
> 
> Some quick calculations:
> For waking up, we need to switch on the main oscillator, wait until it
> stabilizes (default should be max: ~60ms), switch on plla, wait until it
> locks (2ms), switch on pllb, wait until it locks (2ms), switch system to PLLA...
> And then set up the drivers again... On 115200 baud, this is almost 1kB!
> 
> So in many applications we could not use this. But this might still come handy
> in a lot of cases we can poll and find out what caused the data on the serial
> port etc. Or on applications, where this loss of data does not matter (like 
> debug
> console where the resume is usable even if it does not wake up on the first 
> byte).

Of course, but it is the same for all devices when you do a suspend with a 
slow clock.
 
> 
> 
> -- 
> Anti Sullin
> Embedded Software Engineer
> Artec Design LLC
> Akadeemia tee 23A, 12618, Tallinn, Estonia
> http://www.artecdesign.ee 

Regards Michael

__________________________________________________
Do You Yahoo!?
Poco spazio e tanto spam? Yahoo! Mail ti protegge dallo spam e ti da tanto spazio gratuito per i tuoi file e i messaggi 
http://mail.yahoo.it 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] atmel_serial: update the powersave handler to match serial core
  2008-09-19 21:49   ` Anti Sullin
@ 2008-09-20 13:01     ` Haavard Skinnemoen
  0 siblings, 0 replies; 12+ messages in thread
From: Haavard Skinnemoen @ 2008-09-20 13:01 UTC (permalink / raw)
  To: Anti Sullin; +Cc: Michael Trimarchi, linux-serial, linux-kernel, Andrew Victor

Anti Sullin <anti.sullin@artecdesign.ee> wrote:
> Haavard Skinnemoen wrote:
> > I don't think the gpio layer is supposed to touch the portmux. David
> > has always been very clear about that. But if we somehow manage to get
> > the pin configured as a GPIO, we can use the GPIO layer to request a
> > pin change interrupt.
> >
> > It _might_ work even if you don't reconfigure the pin as a GPIO...but
> > then I think we'd be relying on undocumented behaviour.
> >   
> I believe that you don't need to reconfigure the pin. The gpio hw does
> not require the pin to be multiplexed to any given state, so does not request_irq (correct?).
> I did this trick in my [2/3] MCI driver patch I sent to arm-linux-kernel at 18.03.2008 (the
> e-mail subject line was wrong, [1/3], though) and I'm using that patch still on my
> production devices to avoid some rare but critical SD data corruption (mainline kernel still 
> screws up the filesystem on SD sometimes). The same should be easily usable on serial port, too.

You're right. It works, and it's documented. I don't think it's
guaranteed by the GPIO API, but as long as it's guaranteed on AT91 and
AVR32, we should be fine.

> So in many applications we could not use this. But this might still come handy
> in a lot of cases we can poll and find out what caused the data on the serial
> port etc. Or on applications, where this loss of data does not matter (like debug
> console where the resume is usable even if it does not wake up on the first byte).

Yes, as long as you have some sort of timeout/retry mechanism in place,
it might be useful.

Haavard

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2008-09-20 13:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-19 15:11 [PATCH] atmel_serial: update the powersave handler to match serial core Haavard Skinnemoen
2008-09-19 15:19 ` Anti Sullin
2008-09-19 15:39   ` [PATCH v2] " Haavard Skinnemoen
  -- strict thread matches above, loose matches on Subject: below --
2008-09-19 15:33 [PATCH] " Michael Trimarchi
2008-09-19 15:46 ` Haavard Skinnemoen
2008-09-19 16:01 Michael Trimarchi
2008-09-19 16:15 ` Haavard Skinnemoen
2008-09-19 16:33 Michael Trimarchi
2008-09-19 18:35 ` Haavard Skinnemoen
2008-09-19 21:49   ` Anti Sullin
2008-09-20 13:01     ` Haavard Skinnemoen
2008-09-20  9:31 Michael Trimarchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).