From: Felipe Balbi <hidden> Date: 2012-08-21 09:19:59
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
cheers
Felipe Balbi (13):
serial: omap: define and use to_uart_omap_port()
serial: omap: always return IRQ_HANDLED
serial: omap: define helpers for pdata function pointers
serial: omap: don't access the platform_device
serial: omap: drop DMA support
serial: add OMAP-specific defines
serial: omap: simplify IRQ handling
serial: omap: refactor receive_chars() into rdi/rlsi handlers
serial: omap: move THRE check to transmit_chars()
serial: omap: stick to put_autosuspend
serial: omap: set dev->drvdata before enabling pm_runtime
serial: omap: drop unnecessary check from remove
serial: omap: make sure to suspend device before remove
arch/arm/mach-omap2/serial.c | 15 +-
arch/arm/plat-omap/include/plat/omap-serial.h | 12 +-
drivers/tty/serial/omap-serial.c | 707 +++++++++-----------------
include/linux/serial_reg.h | 4 +
4 files changed, 250 insertions(+), 488 deletions(-)
--
1.7.12.rc3
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:08
current code only works because struct uart_port
is the first member on the uart_omap_port structure.
If, for whatever reason, someone puts another
member as the first of the structure, that cast
won't work anymore. In order to be safe, let's use
a container_of() which, for now, gets optimized into
a cast anyway.
Signed-off-by: Felipe Balbi <redacted>
---
arch/arm/plat-omap/include/plat/omap-serial.h | 2 ++
drivers/tty/serial/omap-serial.c | 36 +++++++++++++--------------
2 files changed, 20 insertions(+), 18 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:14
this patch is in preparation to a few other changes
which will align on the prototype for function
pointers passed through pdata.
It also helps cleaning up the driver a little by
agregating checks for pdata in a single location.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 66 ++++++++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 19 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:23
OMAP has some extra Interrupt types which can
be really useful for SW. Let's define them
so we can later use those in OMAP's serial driver.
Signed-off-by: Felipe Balbi <redacted>
---
include/linux/serial_reg.h | 4 ++++
1 file changed, 4 insertions(+)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:28
quite a few changes here, though they are
pretty obvious. In summary we're making sure
to detect which interrupt type we need to
handle before calling the underlying interrupt
handling procedure.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 47 ++++++++++++++++++++++++++++++----------
1 file changed, 35 insertions(+), 12 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:38
by the time we call our first pm_runtme_get_sync()
after enable pm_runtime, our resume method might
be called. To avoid problems, we must make sure
that our dev->drvdata is set correctly before
our resume method gets called.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:20:41
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:21:20
before removing the driver, let's make sure
to force device into a suspended state in order
to conserve power.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 1 +
1 file changed, 1 insertion(+)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:21:45
if platform_get_drvdata() returns NULL, that's
quite a nasty bug on the driver which we want to
catch ASAP. Otherwise, that check is hugely
unneeded.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:22:08
since all other IRQ types now do all necessary
checks inside their handlers, transmit_chars()
was the only one left expecting serial_omap_irq()
to check THRE for it. We can move THRE check to
transmit_chars() in order to make serial_omap_irq()
more uniform.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 09:22:11
receive_chars() was getting too big and too difficult
to follow. By splitting it into separate RDI and RSLI
handlers, we have smaller functions which are easy
to understand and only touch the pieces which they need
to touch.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 203 +++++++++++++++++++--------------------
1 file changed, 100 insertions(+), 103 deletions(-)
@@ -195,74 +195,6 @@ static void serial_omap_stop_rx(struct uart_port *port)pm_runtime_put_autosuspend(up->dev);}-staticinlinevoidreceive_chars(structuart_omap_port*up,-unsignedint*status)-{-structtty_struct*tty=up->port.state->port.tty;-unsignedintflag,lsr=*status;-unsignedcharch=0;-intmax_count=256;--do{-if(likely(lsr&UART_LSR_DR))-ch=serial_in(up,UART_RX);-flag=TTY_NORMAL;-up->port.icount.rx++;--if(unlikely(lsr&UART_LSR_BRK_ERROR_BITS)){-/*-*Forstatisticsonly-*/-if(lsr&UART_LSR_BI){-lsr&=~(UART_LSR_FE|UART_LSR_PE);-up->port.icount.brk++;-/*-*WedotheSysRQandSAKchecking-*herebecauseotherwisethebreak-*maygetmaskedbyignore_status_mask-*orread_status_mask.-*/-if(uart_handle_break(&up->port))-gotoignore_char;-}elseif(lsr&UART_LSR_PE){-up->port.icount.parity++;-}elseif(lsr&UART_LSR_FE){-up->port.icount.frame++;-}--if(lsr&UART_LSR_OE)-up->port.icount.overrun++;--/*-*Maskoffconditionswhichshouldbeignored.-*/-lsr&=up->port.read_status_mask;--#ifdef CONFIG_SERIAL_OMAP_CONSOLE-if(up->port.line==up->port.cons->index){-/* Recover the break flag from console xmit */-lsr|=up->lsr_break_flag;-}-#endif-if(lsr&UART_LSR_BI)-flag=TTY_BREAK;-elseif(lsr&UART_LSR_PE)-flag=TTY_PARITY;-elseif(lsr&UART_LSR_FE)-flag=TTY_FRAME;-}--if(uart_handle_sysrq_char(&up->port,ch))-gotoignore_char;-uart_insert_char(&up->port,lsr,UART_LSR_OE,ch,flag);-ignore_char:-lsr=serial_in(up,UART_LSR);-}while((lsr&(UART_LSR_DR|UART_LSR_BI))&&(max_count-->0));-spin_unlock(&up->port.lock);-tty_flip_buffer_push(tty);-spin_lock(&up->port.lock);-}-staticvoidtransmit_chars(structuart_omap_port*up){structcirc_buf*xmit=&up->port.state->xmit;
@@ -341,6 +273,68 @@ static unsigned int check_modem_status(struct uart_omap_port *up)returnstatus;}+staticvoidserial_omap_rlsi(structuart_omap_port*up,unsignedintlsr)+{+unsignedintflag;++up->port.icount.rx++;+flag=TTY_NORMAL;++if(lsr&UART_LSR_BI){+flag=TTY_BREAK;+lsr&=~(UART_LSR_FE|UART_LSR_PE);+up->port.icount.brk++;+/*+*WedotheSysRQandSAKchecking+*herebecauseotherwisethebreak+*maygetmaskedbyignore_status_mask+*orread_status_mask.+*/+if(uart_handle_break(&up->port))+return;++}++if(lsr&UART_LSR_PE){+flag=TTY_PARITY;+up->port.icount.parity++;+}++if(lsr&UART_LSR_FE){+flag=TTY_FRAME;+up->port.icount.frame++;+}++if(lsr&UART_LSR_OE)+up->port.icount.overrun++;++#ifdef CONFIG_SERIAL_OMAP_CONSOLE+if(up->port.line==up->port.cons->index){+/* Recover the break flag from console xmit */+lsr|=up->lsr_break_flag;+}+#endif+uart_insert_char(&up->port,lsr,UART_LSR_OE,0,flag);+}++staticvoidserial_omap_rdi(structuart_omap_port*up,unsignedintlsr)+{+unsignedcharch=0;+unsignedintflag;++if(!(lsr&UART_LSR_DR))+return;++ch=serial_in(up,UART_RX);+flag=TTY_NORMAL;+up->port.icount.rx++;++if(uart_handle_sysrq_char(&up->port,ch))+return;++uart_insert_char(&up->port,lsr,UART_LSR_OE,ch,flag);+}+/***serial_omap_irq()-Thishandlestheinterruptfromoneport*@irq:uartportirqnumber
@@ -349,52 +343,55 @@ static unsigned int check_modem_status(struct uart_omap_port *up)staticinlineirqreturn_tserial_omap_irq(intirq,void*dev_id){structuart_omap_port*up=dev_id;+structtty_struct*tty=up->port.state->port.tty;unsignedintiir,lsr;unsignedinttype;unsignedlongflags;+intmax_count=256;spin_lock_irqsave(&up->port.lock,flags);pm_runtime_get_sync(up->dev);-iir=serial_in(up,UART_IIR);-again:-if(iir&UART_IIR_NO_INT)-gotoout;-lsr=serial_in(up,UART_LSR);+do{+iir=serial_in(up,UART_IIR);+if(iir&UART_IIR_NO_INT)+break;-/* extract IRQ type from IIR register */-type=iir&0x3e;+lsr=serial_in(up,UART_LSR);-switch(type){-caseUART_IIR_MSI:-check_modem_status(up);-break;-caseUART_IIR_THRI:-if(lsr&UART_LSR_THRE)-transmit_chars(up);-break;-caseUART_IIR_RDI:-if(lsr&UART_LSR_DR)-receive_chars(up,&lsr);-break;-caseUART_IIR_RLSI:-if(lsr&UART_LSR_BRK_ERROR_BITS)-receive_chars(up,&lsr);-break;-caseUART_IIR_RX_TIMEOUT:-receive_chars(up,&lsr);-break;-caseUART_IIR_CTS_RTS_DSR:-iir=serial_in(up,UART_IIR);-gotoagain;-caseUART_IIR_XOFF:-/* FALLTHROUGH */-default:-break;-}+/* extract IRQ type from IIR register */+type=iir&0x3e;++switch(type){+caseUART_IIR_MSI:+check_modem_status(up);+break;+caseUART_IIR_THRI:+if(lsr&UART_LSR_THRE)+transmit_chars(up);+break;+caseUART_IIR_RX_TIMEOUT:+/* FALLTHROUGH */+caseUART_IIR_RDI:+serial_omap_rdi(up,lsr);+break;+caseUART_IIR_RLSI:+serial_omap_rlsi(up,lsr);+break;+caseUART_IIR_CTS_RTS_DSR:+/* simply try again */+break;+caseUART_IIR_XOFF:+/* FALLTHROUGH */+default:+break;+}+}while(!(iir&UART_IIR_NO_INT)&&max_count--);-out:spin_unlock_irqrestore(&up->port.lock,flags);++tty_flip_buffer_push(tty);+pm_runtime_mark_last_busy(up->dev);pm_runtime_put_autosuspend(up->dev);up->port_activity=jiffies;
From: Felipe Balbi <hidden> Date: 2012-08-21 09:22:15
The current support is known to be broken and
a later patch will come re-adding it using
dma engine API.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 330 ++-------------------------------------
1 file changed, 12 insertions(+), 318 deletions(-)
@@ -605,20 +505,6 @@ static int serial_omap_startup(struct uart_port *port)spin_unlock_irqrestore(&up->port.lock,flags);up->msr_saved_flags=0;-if(up->use_dma){-free_page((unsignedlong)up->port.state->xmit.buf);-up->port.state->xmit.buf=dma_alloc_coherent(NULL,-UART_XMIT_SIZE,-(dma_addr_t*)&(up->uart_dma.tx_buf_dma_phys),-0);-init_timer(&(up->uart_dma.rx_timer));-up->uart_dma.rx_timer.function=serial_omap_rxdma_poll;-up->uart_dma.rx_timer.data=up->port.line;-/* Currently the buffer size is 4KB. Can increase it */-up->uart_dma.rx_buf=dma_alloc_coherent(NULL,-up->uart_dma.rx_buf_size,-(dma_addr_t*)&(up->uart_dma.rx_buf_dma_phys),0);-}/**Finally,enableinterrupts.Note:Modemstatusinterrupts*aresetviaset_termios(),whichwillbeoccurringimminently
From: Felipe Balbi <hidden> Date: 2012-08-21 09:23:14
The driver doesn't need to know about its platform_device.
Everything the driver needs can be done through the
struct device pointer. In case we need to use the
OMAP-specific PM function pointers, those can make
sure to find the device's platform_device pointer
so they can find the struct omap_device through
pdev->archdata field.
Signed-off-by: Felipe Balbi <redacted>
---
arch/arm/mach-omap2/serial.c | 15 ++--
arch/arm/plat-omap/include/plat/omap-serial.h | 10 +--
drivers/tty/serial/omap-serial.c | 124 +++++++++++++-------------
3 files changed, 76 insertions(+), 73 deletions(-)
@@ -574,7 +574,7 @@ static int serial_omap_startup(struct uart_port *port)dev_dbg(up->port.dev,"serial_omap_startup+%d\n",up->port.line);-pm_runtime_get_sync(&up->pdev->dev);+pm_runtime_get_sync(up->dev);/**CleartheFIFObuffersanddisablethem.*(theywillbereenabledinset_termios())
@@ -630,8 +630,8 @@ static int serial_omap_startup(struct uart_port *port)/* Enable module level wake up */serial_out(up,UART_OMAP_WER,OMAP_UART_WER_MOD_WKUP);-pm_runtime_mark_last_busy(&up->pdev->dev);-pm_runtime_put_autosuspend(&up->pdev->dev);+pm_runtime_mark_last_busy(up->dev);+pm_runtime_put_autosuspend(up->dev);up->port_activity=jiffies;return0;}
From: Felipe Balbi <hidden> Date: 2012-08-21 09:23:57
Even if we enter our IRQ handler just to notice
that the our device didn't generate the IRQ,
that still means "handling" and IRQ, so let's
return IRQ_HANDLED.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 10:24:47
On Tue, Aug 21, 2012 at 03:14:19PM +0530, Shilimkar, Santosh wrote:
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
The current support is known to be broken and
a later patch will come re-adding it using
dma engine API.
Signed-off-by: Felipe Balbi <redacted>
---
Thanks Felipe !!
no problem.
One less driver now towards OMAP DMA
engine conversion.
indeed :-) I'll take a closer look into rx timeout IRQ, but it looks
like we can use it to kick dma only for "big" transfers... need to play
with it for a while first, though.
On Tue, Aug 21, 2012 at 3:50 PM, Felipe Balbi [off-list ref] wrote:
On Tue, Aug 21, 2012 at 03:14:19PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
The current support is known to be broken and
a later patch will come re-adding it using
dma engine API.
Signed-off-by: Felipe Balbi <redacted>
---
Thanks Felipe !!
no problem.
quoted
One less driver now towards OMAP DMA
engine conversion.
indeed :-) I'll take a closer look into rx timeout IRQ, but it looks
like we can use it to kick dma only for "big" transfers... need to play
with it for a while first, though.
Yep. The RX path with DMA is bit of difficult part to manage for
UART.
quoted
FWIW,
Acked-by: Santosh Shilimkar <redacted>
is this Ack for this patch only or the entire series ??
Two more patches to review and then I will do it for
full series on top of the cover-letter :-)
Regards
Santosh
From: Felipe Balbi <hidden> Date: 2012-08-21 10:38:41
On Tue, Aug 21, 2012 at 04:05:22PM +0530, Shilimkar, Santosh wrote:
On Tue, Aug 21, 2012 at 3:50 PM, Felipe Balbi [off-list ref] wrote:
quoted
On Tue, Aug 21, 2012 at 03:14:19PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
The current support is known to be broken and
a later patch will come re-adding it using
dma engine API.
Signed-off-by: Felipe Balbi <redacted>
---
Thanks Felipe !!
no problem.
quoted
One less driver now towards OMAP DMA
engine conversion.
indeed :-) I'll take a closer look into rx timeout IRQ, but it looks
like we can use it to kick dma only for "big" transfers... need to play
with it for a while first, though.
Yep. The RX path with DMA is bit of difficult part to manage for
UART.
quoted
quoted
FWIW,
Acked-by: Santosh Shilimkar <redacted>
is this Ack for this patch only or the entire series ??
Two more patches to review and then I will do it for
full series on top of the cover-letter :-)
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted hunk
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
cheers
Felipe Balbi (13):
serial: omap: define and use to_uart_omap_port()
serial: omap: always return IRQ_HANDLED
serial: omap: define helpers for pdata function pointers
serial: omap: don't access the platform_device
serial: omap: drop DMA support
serial: add OMAP-specific defines
serial: omap: simplify IRQ handling
serial: omap: refactor receive_chars() into rdi/rlsi handlers
serial: omap: move THRE check to transmit_chars()
serial: omap: stick to put_autosuspend
serial: omap: set dev->drvdata before enabling pm_runtime
serial: omap: drop unnecessary check from remove
serial: omap: make sure to suspend device before remove
Apart from that one question on last patch, rest of the clean-up
is really good. Nice work.
FWIW,
Acked-by: Santosh Shilimkar <redacted>
From: Felipe Balbi <hidden> Date: 2012-08-21 11:01:05
On Tue, Aug 21, 2012 at 04:12:11PM +0530, Shilimkar, Santosh wrote:
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
Can you please expand the change-log a bit ?
Didn't follow the time re-init part completely.
It's really just a micro-optimization. The thing is:
if I call pm_runtime_put(), I will not reinitialize the pm timer to
whatever timeout value I used. This means that pm_runtime_put() could
actually execute right away (if timer was about to expire when I called
pm_runtime_put()). While this wouldn't cause any issues, it's better to
reinitialize the timer and make sure if there's another
read/write/set_termios/whatever coming right after this, UART is still
powered up.
I mean, it's really just trying to avoid context save & restore when
UART is still under heavy usage.
Does it make sense ?
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/d67495a8/attachment-0001.sig>
On Tue, Aug 21, 2012 at 4:27 PM, Felipe Balbi [off-list ref] wrote:
On Tue, Aug 21, 2012 at 04:12:11PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
Can you please expand the change-log a bit ?
Didn't follow the time re-init part completely.
It's really just a micro-optimization. The thing is:
if I call pm_runtime_put(), I will not reinitialize the pm timer to
whatever timeout value I used. This means that pm_runtime_put() could
actually execute right away (if timer was about to expire when I called
pm_runtime_put()). While this wouldn't cause any issues, it's better to
reinitialize the timer and make sure if there's another
read/write/set_termios/whatever coming right after this, UART is still
powered up.
I mean, it's really just trying to avoid context save & restore when
UART is still under heavy usage.
Does it make sense ?
It does. Would be good to add the above description in the change-log.
Thanks for clarification.
Regars
Santosh
From: Felipe Balbi <hidden> Date: 2012-08-21 11:06:50
On Tue, Aug 21, 2012 at 04:35:26PM +0530, Shilimkar, Santosh wrote:
On Tue, Aug 21, 2012 at 4:27 PM, Felipe Balbi [off-list ref] wrote:
quoted
On Tue, Aug 21, 2012 at 04:12:11PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
Can you please expand the change-log a bit ?
Didn't follow the time re-init part completely.
It's really just a micro-optimization. The thing is:
if I call pm_runtime_put(), I will not reinitialize the pm timer to
whatever timeout value I used. This means that pm_runtime_put() could
actually execute right away (if timer was about to expire when I called
pm_runtime_put()). While this wouldn't cause any issues, it's better to
reinitialize the timer and make sure if there's another
read/write/set_termios/whatever coming right after this, UART is still
powered up.
I mean, it's really just trying to avoid context save & restore when
UART is still under heavy usage.
Does it make sense ?
It does. Would be good to add the above description in the change-log.
Thanks for clarification.
From: Felipe Balbi <hidden> Date: 2012-08-21 11:13:53
Hi,
On Tue, Aug 21, 2012 at 02:02:46PM +0300, Felipe Balbi wrote:
On Tue, Aug 21, 2012 at 04:35:26PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 4:27 PM, Felipe Balbi [off-list ref] wrote:
quoted
On Tue, Aug 21, 2012 at 04:12:11PM +0530, Shilimkar, Santosh wrote:
quoted
On Tue, Aug 21, 2012 at 2:45 PM, Felipe Balbi [off-list ref] wrote:
quoted
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
Can you please expand the change-log a bit ?
Didn't follow the time re-init part completely.
It's really just a micro-optimization. The thing is:
if I call pm_runtime_put(), I will not reinitialize the pm timer to
whatever timeout value I used. This means that pm_runtime_put() could
actually execute right away (if timer was about to expire when I called
pm_runtime_put()). While this wouldn't cause any issues, it's better to
reinitialize the timer and make sure if there's another
read/write/set_termios/whatever coming right after this, UART is still
powered up.
I mean, it's really just trying to avoid context save & restore when
UART is still under heavy usage.
Does it make sense ?
It does. Would be good to add the above description in the change-log.
Thanks for clarification.
will do, cheers
I have updated my branch like below. Will wait for any other comments
before sending another version.
commit 8ff7ab777d2bf8619328ddd43ddf2f8660dd011f
Author: Felipe Balbi [off-list ref]
Date: Tue Aug 21 11:45:47 2012 +0300
serial: omap: stick to put_autosuspend
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
The idea behind this patch is to make sure we
will always reinitialize the pm timer so that
we don't fall into a situation where pm_runtime_put()
expires right away (if timer was already about to
expire when we made the call to pm_runtime_put()).
While suspending right away wouldn't cause any
issues, reinitializing the pm timer can help us
avoiding unnecessary context save & restore
operations (which are somewhat expensive) if there's
another read/write/set_termios request coming right
after. IOW, we are trying to make sure UART is still
powered up while it's still under heavy usage.
Acked-by: Santosh Shilimkar [off-list ref]
Signed-off-by: Felipe Balbi [off-list ref]
On Tue, 21 Aug 2012 12:15:44 +0300
Felipe Balbi [off-list ref] wrote:
Even if we enter our IRQ handler just to notice
that the our device didn't generate the IRQ,
that still means "handling" and IRQ, so let's
return IRQ_HANDLED.
That looks wrong - you'll defeat the stuck IRQ protection. If we didn't
cause the IRQ then we are IRQ_NONE ?
From: Felipe Balbi <hidden> Date: 2012-08-21 11:58:28
On Tue, Aug 21, 2012 at 12:50:05PM +0100, Alan Cox wrote:
On Tue, 21 Aug 2012 12:15:44 +0300
Felipe Balbi [off-list ref] wrote:
quoted
Even if we enter our IRQ handler just to notice
that the our device didn't generate the IRQ,
that still means "handling" and IRQ, so let's
return IRQ_HANDLED.
That looks wrong - you'll defeat the stuck IRQ protection. If we didn't
cause the IRQ then we are IRQ_NONE ?
From: Felipe Balbi <hidden> Date: 2012-08-21 12:20:15
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
Changes since v1:
. improved commit log on patch 9/13 (formerly 10/13)
. removed patch 2/13
. added a new patch switching from spin_lock_irqsave() to spin_lock and
spin_unlock_irqrestore to spin_unlock
Retested with my pandaboard, UART continues to work:
# grep -i uart /proc/interrupts
106: 124 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 189 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 255 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 321 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 387 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 453 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 519 0 GIC OMAP UART2
cheers
ps: if anyone knows a better test for UART, let me know.
for convenience of anyone testing, patches are available on my git tree [1] on
branch uart
[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart
Felipe Balbi (13):
serial: omap: define and use to_uart_omap_port()
serial: omap: define helpers for pdata function pointers
serial: omap: don't access the platform_device
serial: omap: drop DMA support
serial: add OMAP-specific defines
serial: omap: simplify IRQ handling
serial: omap: refactor receive_chars() into rdi/rlsi handlers
serial: omap: move THRE check to transmit_chars()
serial: omap: stick to put_autosuspend
serial: omap: set dev->drvdata before enabling pm_runtime
serial: omap: drop unnecessary check from remove
serial: omap: make sure to suspend device before remove
serial: omap: don't save IRQ flags on hardirq
arch/arm/mach-omap2/serial.c | 15 +-
arch/arm/plat-omap/include/plat/omap-serial.h | 12 +-
drivers/tty/serial/omap-serial.c | 713 +++++++++-----------------
include/linux/serial_reg.h | 4 +
4 files changed, 254 insertions(+), 490 deletions(-)
--
1.7.12.rc3
From: Felipe Balbi <hidden> Date: 2012-08-21 12:20:25
this patch is in preparation to a few other changes
which will align on the prototype for function
pointers passed through pdata.
It also helps cleaning up the driver a little by
agregating checks for pdata in a single location.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 66 ++++++++++++++++++++++++++++------------
1 file changed, 47 insertions(+), 19 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:20:35
OMAP has some extra Interrupt types which can
be really useful for SW. Let's define them
so we can later use those in OMAP's serial driver.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
include/linux/serial_reg.h | 4 ++++
1 file changed, 4 insertions(+)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:20:48
quite a few changes here, though they are
pretty obvious. In summary we're making sure
to detect which interrupt type we need to
handle before calling the underlying interrupt
handling procedure.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 48 +++++++++++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 11 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:20:57
When we're running our hardirq handler, there's
not need to disable IRQs with spin_lock_irqsave()
because IRQs are already disabled. It also makes
no difference if we save or not IRQ flags.
Switch over to simple spin_lock/spin_unlock and
drop the "flags" variable.
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:21:18
before removing the driver, let's make sure
to force device into a suspended state in order
to conserve power.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 1 +
1 file changed, 1 insertion(+)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:21:33
if platform_get_drvdata() returns NULL, that's
quite a nasty bug on the driver which we want to
catch ASAP. Otherwise, that check is hugely
unneeded.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:21:38
by the time we call our first pm_runtme_get_sync()
after enable pm_runtime, our resume method might
be called. To avoid problems, we must make sure
that our dev->drvdata is set correctly before
our resume method gets called.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:21:41
Everytime we're done using our TTY, we want
the pm timer to be reinitilized. By sticking
to pm_runtime_pm_autosuspend() we make sure
that this will always be the case.
The idea behind this patch is to make sure we
will always reinitialize the pm timer so that
we don't fall into a situation where pm_runtime_put()
expires right away (if timer was already about to
expire when we made the call to pm_runtime_put()).
While suspending right away wouldn't cause any
issues, reinitializing the pm timer can help us
avoiding unnecessary context save & restore
operations (which are somewhat expensive) if there's
another read/write/set_termios request coming right
after. IOW, we are trying to make sure UART is still
powered up while it's still under heavy usage.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:22:22
since all other IRQ types now do all necessary
checks inside their handlers, transmit_chars()
was the only one left expecting serial_omap_irq()
to check THRE for it. We can move THRE check to
transmit_chars() in order to make serial_omap_irq()
more uniform.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 12:22:24
receive_chars() was getting too big and too difficult
to follow. By splitting it into separate RDI and RSLI
handlers, we have smaller functions which are easy
to understand and only touch the pieces which they need
to touch.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 207 +++++++++++++++++++--------------------
1 file changed, 102 insertions(+), 105 deletions(-)
@@ -195,74 +195,6 @@ static void serial_omap_stop_rx(struct uart_port *port)pm_runtime_put_autosuspend(up->dev);}-staticinlinevoidreceive_chars(structuart_omap_port*up,-unsignedint*status)-{-structtty_struct*tty=up->port.state->port.tty;-unsignedintflag,lsr=*status;-unsignedcharch=0;-intmax_count=256;--do{-if(likely(lsr&UART_LSR_DR))-ch=serial_in(up,UART_RX);-flag=TTY_NORMAL;-up->port.icount.rx++;--if(unlikely(lsr&UART_LSR_BRK_ERROR_BITS)){-/*-*Forstatisticsonly-*/-if(lsr&UART_LSR_BI){-lsr&=~(UART_LSR_FE|UART_LSR_PE);-up->port.icount.brk++;-/*-*WedotheSysRQandSAKchecking-*herebecauseotherwisethebreak-*maygetmaskedbyignore_status_mask-*orread_status_mask.-*/-if(uart_handle_break(&up->port))-gotoignore_char;-}elseif(lsr&UART_LSR_PE){-up->port.icount.parity++;-}elseif(lsr&UART_LSR_FE){-up->port.icount.frame++;-}--if(lsr&UART_LSR_OE)-up->port.icount.overrun++;--/*-*Maskoffconditionswhichshouldbeignored.-*/-lsr&=up->port.read_status_mask;--#ifdef CONFIG_SERIAL_OMAP_CONSOLE-if(up->port.line==up->port.cons->index){-/* Recover the break flag from console xmit */-lsr|=up->lsr_break_flag;-}-#endif-if(lsr&UART_LSR_BI)-flag=TTY_BREAK;-elseif(lsr&UART_LSR_PE)-flag=TTY_PARITY;-elseif(lsr&UART_LSR_FE)-flag=TTY_FRAME;-}--if(uart_handle_sysrq_char(&up->port,ch))-gotoignore_char;-uart_insert_char(&up->port,lsr,UART_LSR_OE,ch,flag);-ignore_char:-lsr=serial_in(up,UART_LSR);-}while((lsr&(UART_LSR_DR|UART_LSR_BI))&&(max_count-->0));-spin_unlock(&up->port.lock);-tty_flip_buffer_push(tty);-spin_lock(&up->port.lock);-}-staticvoidtransmit_chars(structuart_omap_port*up){structcirc_buf*xmit=&up->port.state->xmit;
@@ -341,6 +273,68 @@ static unsigned int check_modem_status(struct uart_omap_port *up)returnstatus;}+staticvoidserial_omap_rlsi(structuart_omap_port*up,unsignedintlsr)+{+unsignedintflag;++up->port.icount.rx++;+flag=TTY_NORMAL;++if(lsr&UART_LSR_BI){+flag=TTY_BREAK;+lsr&=~(UART_LSR_FE|UART_LSR_PE);+up->port.icount.brk++;+/*+*WedotheSysRQandSAKchecking+*herebecauseotherwisethebreak+*maygetmaskedbyignore_status_mask+*orread_status_mask.+*/+if(uart_handle_break(&up->port))+return;++}++if(lsr&UART_LSR_PE){+flag=TTY_PARITY;+up->port.icount.parity++;+}++if(lsr&UART_LSR_FE){+flag=TTY_FRAME;+up->port.icount.frame++;+}++if(lsr&UART_LSR_OE)+up->port.icount.overrun++;++#ifdef CONFIG_SERIAL_OMAP_CONSOLE+if(up->port.line==up->port.cons->index){+/* Recover the break flag from console xmit */+lsr|=up->lsr_break_flag;+}+#endif+uart_insert_char(&up->port,lsr,UART_LSR_OE,0,flag);+}++staticvoidserial_omap_rdi(structuart_omap_port*up,unsignedintlsr)+{+unsignedcharch=0;+unsignedintflag;++if(!(lsr&UART_LSR_DR))+return;++ch=serial_in(up,UART_RX);+flag=TTY_NORMAL;+up->port.icount.rx++;++if(uart_handle_sysrq_char(&up->port,ch))+return;++uart_insert_char(&up->port,lsr,UART_LSR_OE,ch,flag);+}+/***serial_omap_irq()-Thishandlestheinterruptfromoneport*@irq:uartportirqnumber
@@ -349,55 +343,58 @@ static unsigned int check_modem_status(struct uart_omap_port *up)staticinlineirqreturn_tserial_omap_irq(intirq,void*dev_id){structuart_omap_port*up=dev_id;+structtty_struct*tty=up->port.state->port.tty;unsignedintiir,lsr;unsignedinttype;unsignedlongflags;irqreturn_tret=IRQ_HANDLED;+intmax_count=256;spin_lock_irqsave(&up->port.lock,flags);pm_runtime_get_sync(up->dev);-iir=serial_in(up,UART_IIR);-again:-if(iir&UART_IIR_NO_INT){-ret=IRQ_NONE;-gotoout;-}-lsr=serial_in(up,UART_LSR);+do{+iir=serial_in(up,UART_IIR);+if(iir&UART_IIR_NO_INT){+ret=IRQ_NONE;+break;+}-/* extract IRQ type from IIR register */-type=iir&0x3e;+lsr=serial_in(up,UART_LSR);-switch(type){-caseUART_IIR_MSI:-check_modem_status(up);-break;-caseUART_IIR_THRI:-if(lsr&UART_LSR_THRE)-transmit_chars(up);-break;-caseUART_IIR_RDI:-if(lsr&UART_LSR_DR)-receive_chars(up,&lsr);-break;-caseUART_IIR_RLSI:-if(lsr&UART_LSR_BRK_ERROR_BITS)-receive_chars(up,&lsr);-break;-caseUART_IIR_RX_TIMEOUT:-receive_chars(up,&lsr);-break;-caseUART_IIR_CTS_RTS_DSR:-iir=serial_in(up,UART_IIR);-gotoagain;-caseUART_IIR_XOFF:-/* FALLTHROUGH */-default:-break;-}+/* extract IRQ type from IIR register */+type=iir&0x3e;++switch(type){+caseUART_IIR_MSI:+check_modem_status(up);+break;+caseUART_IIR_THRI:+if(lsr&UART_LSR_THRE)+transmit_chars(up);+break;+caseUART_IIR_RX_TIMEOUT:+/* FALLTHROUGH */+caseUART_IIR_RDI:+serial_omap_rdi(up,lsr);+break;+caseUART_IIR_RLSI:+serial_omap_rlsi(up,lsr);+break;+caseUART_IIR_CTS_RTS_DSR:+/* simply try again */+break;+caseUART_IIR_XOFF:+/* FALLTHROUGH */+default:+break;+}+}while(!(iir&UART_IIR_NO_INT)&&max_count--);-out:spin_unlock_irqrestore(&up->port.lock,flags);++tty_flip_buffer_push(tty);+pm_runtime_mark_last_busy(up->dev);pm_runtime_put_autosuspend(up->dev);up->port_activity=jiffies;
From: Felipe Balbi <hidden> Date: 2012-08-21 12:23:01
The current support is known to be broken and
a later patch will come re-adding it using
dma engine API.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
drivers/tty/serial/omap-serial.c | 330 ++-------------------------------------
1 file changed, 12 insertions(+), 318 deletions(-)
@@ -605,20 +505,6 @@ static int serial_omap_startup(struct uart_port *port)spin_unlock_irqrestore(&up->port.lock,flags);up->msr_saved_flags=0;-if(up->use_dma){-free_page((unsignedlong)up->port.state->xmit.buf);-up->port.state->xmit.buf=dma_alloc_coherent(NULL,-UART_XMIT_SIZE,-(dma_addr_t*)&(up->uart_dma.tx_buf_dma_phys),-0);-init_timer(&(up->uart_dma.rx_timer));-up->uart_dma.rx_timer.function=serial_omap_rxdma_poll;-up->uart_dma.rx_timer.data=up->port.line;-/* Currently the buffer size is 4KB. Can increase it */-up->uart_dma.rx_buf=dma_alloc_coherent(NULL,-up->uart_dma.rx_buf_size,-(dma_addr_t*)&(up->uart_dma.rx_buf_dma_phys),0);-}/**Finally,enableinterrupts.Note:Modemstatusinterrupts*aresetviaset_termios(),whichwillbeoccurringimminently
From: Felipe Balbi <hidden> Date: 2012-08-21 12:23:56
The driver doesn't need to know about its platform_device.
Everything the driver needs can be done through the
struct device pointer. In case we need to use the
OMAP-specific PM function pointers, those can make
sure to find the device's platform_device pointer
so they can find the struct omap_device through
pdev->archdata field.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
arch/arm/mach-omap2/serial.c | 15 ++--
arch/arm/plat-omap/include/plat/omap-serial.h | 10 +--
drivers/tty/serial/omap-serial.c | 124 +++++++++++++-------------
3 files changed, 76 insertions(+), 73 deletions(-)
@@ -574,7 +574,7 @@ static int serial_omap_startup(struct uart_port *port)dev_dbg(up->port.dev,"serial_omap_startup+%d\n",up->port.line);-pm_runtime_get_sync(&up->pdev->dev);+pm_runtime_get_sync(up->dev);/**CleartheFIFObuffersanddisablethem.*(theywillbereenabledinset_termios())
@@ -630,8 +630,8 @@ static int serial_omap_startup(struct uart_port *port)/* Enable module level wake up */serial_out(up,UART_OMAP_WER,OMAP_UART_WER_MOD_WKUP);-pm_runtime_mark_last_busy(&up->pdev->dev);-pm_runtime_put_autosuspend(&up->pdev->dev);+pm_runtime_mark_last_busy(up->dev);+pm_runtime_put_autosuspend(up->dev);up->port_activity=jiffies;return0;}
From: Felipe Balbi <hidden> Date: 2012-08-21 12:23:59
current code only works because struct uart_port
is the first member on the uart_omap_port structure.
If, for whatever reason, someone puts another
member as the first of the structure, that cast
won't work anymore. In order to be safe, let's use
a container_of() which, for now, gets optimized into
a cast anyway.
Acked-by: Santosh Shilimkar <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
arch/arm/plat-omap/include/plat/omap-serial.h | 2 ++
drivers/tty/serial/omap-serial.c | 36 +++++++++++++--------------
2 files changed, 20 insertions(+), 18 deletions(-)
From: Felipe Balbi <hidden> Date: 2012-08-21 13:06:07
Hi,
On Tue, Aug 21, 2012 at 03:15:58PM +0300, Felipe Balbi wrote:
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
Changes since v1:
. improved commit log on patch 9/13 (formerly 10/13)
. removed patch 2/13
. added a new patch switching from spin_lock_irqsave() to spin_lock and
spin_unlock_irqrestore to spin_unlock
Retested with my pandaboard, UART continues to work:
# grep -i uart /proc/interrupts
106: 124 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 189 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 255 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 321 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 387 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 453 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 519 0 GIC OMAP UART2
cheers
ps: if anyone knows a better test for UART, let me know.
for convenience of anyone testing, patches are available on my git tree [1] on
branch uart
[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart
I have added one extra patch to this series:
From 6921efdb13dda7af216b331bb35535d4b53f004a Mon Sep 17 00:00:00 2001
From: Felipe Balbi <redacted>
Date: Tue, 21 Aug 2012 15:48:35 +0300
Subject: [PATCH] serial: omap: drop pm_runtime_irq_safe() usage
pm_runtime_irq_safe() will essentially do an
unbalanced pm_runtime_get_sync() on our parent
device, which might cause problems when trying
to suspend.
In order to prevent that we drop pm_runtime_irq_safe
usage in exchange for a little performance hit
when we enter our IRQ handler while still suspended.
If that happens, what we will do is set the irq_pending
flag, do an asynchronous pm_runtime_get() call and
return IRQ_HANDLED. When our runtime_resume() callback
is executed, we check for that flag, and run our
IRQ handler so that we receive/transmit the pending
characters.
Signed-off-by: Felipe Balbi <redacted>
---
One extra patch to OMAP UART driver. This seems to be working
pretty well even after echo mem > /sys/power/state. I can
see that I can even wake my pandaboard up by sending a character
through serial:
# echo mem > /sys/power/state
[ 1335.679260] PM: Syncing filesystems ... done.
[ 1335.684387] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 1335.691741] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
[ 1335.720886] Suspending console(s) (use no_console_suspend to debug)
[ 1335.734405] PM: suspend of devices complete after 5.492 msecs
[ 1335.735534] PM: late suspend of devices complete after 1.128 msecs
[ 1335.737518] PM: noirq suspend of devices complete after 1.952 msecs
[ 1335.737518] Disabling non-boot CPUs ...
[ 1335.738525] CPU1: shutdown
[ 1338.543762] Successfully put all powerdomains to target state
[ 1338.543853] Enabling non-boot CPUs ...
[ 1338.545654] CPU1: Booted secondary processor
[ 1338.546020] CPU1 is up
[ 1338.547027] PM: noirq resume of devices complete after 0.976 msecs
[ 1338.548400] PM: early resume of devices complete after 0.762 msecs
[ 1339.827087] PM: resume of devices complete after 1278.686 msecs
[ 1339.890960] Restarting tasks ... done.
# # grep -i uart /proc/interrupts
106: 3385 0 GIC OMAP UART2
# echo mem > /sys/power/state
[ 1358.015624] PM: Syncing filesystems ... done.
[ 1358.020812] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 1358.028167] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 1358.055084] Suspending console(s) (use no_console_suspend to debug)
[ 1358.068847] PM: suspend of devices complete after 5.633 msecs
[ 1358.070007] PM: late suspend of devices complete after 1.126 msecs
[ 1358.072051] PM: noirq suspend of devices complete after 2.040 msecs
[ 1358.072051] Disabling non-boot CPUs ...
[ 1358.073211] CPU1: shutdown
[ 1359.104156] Successfully put all powerdomains to target state
[ 1359.104278] Enabling non-boot CPUs ...
[ 1359.106079] CPU1: Booted secondary processor
[ 1359.106475] CPU1 is up
[ 1359.107482] PM: noirq resume of devices complete after 1.004 msecs
[ 1359.108886] PM: early resume of devices complete after 0.761 msecs
[ 1360.414794] PM: resume of devices complete after 1305.836 msecs
[ 1360.478668] Restarting tasks ... done.
# # grep -i uart /proc/interrupts
106: 3511 0 GIC OMAP UART2
arch/arm/plat-omap/include/plat/omap-serial.h | 1 +
drivers/tty/serial/omap-serial.c | 16 +++++++++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
@@ -1305,7 +1310,6 @@ static int serial_omap_probe(struct platform_device *pdev)pm_runtime_set_autosuspend_delay(&pdev->dev,omap_up_info->autosuspend_timeout);-pm_runtime_irq_safe(&pdev->dev);pm_runtime_enable(&pdev->dev);pm_runtime_get_sync(&pdev->dev);
@@ -1416,6 +1420,9 @@ static int serial_omap_runtime_suspend(struct device *dev)if(!up)return-EINVAL;+if(up->irq_pending)+return-EBUSY;+if(!pdata)return0;
@@ -1452,6 +1459,9 @@ static int serial_omap_runtime_resume(struct device *dev)up->latency=up->calc_latency;schedule_work(&up->qos_work);++if(up->irq_pending)+serial_omap_irq(up->port.irq,up);}return0;
--
1.7.12.rc3
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/2d02391b/attachment-0001.sig>
From: Felipe Balbi <hidden> Date: 2012-08-21 15:11:11
Hi,
On Tue, Aug 21, 2012 at 04:01:36PM +0300, Felipe Balbi wrote:
quoted hunk
Hi,
On Tue, Aug 21, 2012 at 03:15:58PM +0300, Felipe Balbi wrote:
quoted
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
Changes since v1:
. improved commit log on patch 9/13 (formerly 10/13)
. removed patch 2/13
. added a new patch switching from spin_lock_irqsave() to spin_lock and
spin_unlock_irqrestore to spin_unlock
Retested with my pandaboard, UART continues to work:
# grep -i uart /proc/interrupts
106: 124 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 189 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 255 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 321 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 387 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 453 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 519 0 GIC OMAP UART2
cheers
ps: if anyone knows a better test for UART, let me know.
for convenience of anyone testing, patches are available on my git tree [1] on
branch uart
[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart
I have added one extra patch to this series:
From 6921efdb13dda7af216b331bb35535d4b53f004a Mon Sep 17 00:00:00 2001
From: Felipe Balbi <redacted>
Date: Tue, 21 Aug 2012 15:48:35 +0300
Subject: [PATCH] serial: omap: drop pm_runtime_irq_safe() usage
pm_runtime_irq_safe() will essentially do an
unbalanced pm_runtime_get_sync() on our parent
device, which might cause problems when trying
to suspend.
In order to prevent that we drop pm_runtime_irq_safe
usage in exchange for a little performance hit
when we enter our IRQ handler while still suspended.
If that happens, what we will do is set the irq_pending
flag, do an asynchronous pm_runtime_get() call and
return IRQ_HANDLED. When our runtime_resume() callback
is executed, we check for that flag, and run our
IRQ handler so that we receive/transmit the pending
characters.
Signed-off-by: Felipe Balbi <redacted>
---
One extra patch to OMAP UART driver. This seems to be working
pretty well even after echo mem > /sys/power/state. I can
see that I can even wake my pandaboard up by sending a character
through serial:
# echo mem > /sys/power/state
[ 1335.679260] PM: Syncing filesystems ... done.
[ 1335.684387] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 1335.691741] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
[ 1335.720886] Suspending console(s) (use no_console_suspend to debug)
[ 1335.734405] PM: suspend of devices complete after 5.492 msecs
[ 1335.735534] PM: late suspend of devices complete after 1.128 msecs
[ 1335.737518] PM: noirq suspend of devices complete after 1.952 msecs
[ 1335.737518] Disabling non-boot CPUs ...
[ 1335.738525] CPU1: shutdown
[ 1338.543762] Successfully put all powerdomains to target state
[ 1338.543853] Enabling non-boot CPUs ...
[ 1338.545654] CPU1: Booted secondary processor
[ 1338.546020] CPU1 is up
[ 1338.547027] PM: noirq resume of devices complete after 0.976 msecs
[ 1338.548400] PM: early resume of devices complete after 0.762 msecs
[ 1339.827087] PM: resume of devices complete after 1278.686 msecs
[ 1339.890960] Restarting tasks ... done.
# # grep -i uart /proc/interrupts
106: 3385 0 GIC OMAP UART2
# echo mem > /sys/power/state
[ 1358.015624] PM: Syncing filesystems ... done.
[ 1358.020812] Freezing user space processes ... (elapsed 0.00 seconds) done.
[ 1358.028167] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 1358.055084] Suspending console(s) (use no_console_suspend to debug)
[ 1358.068847] PM: suspend of devices complete after 5.633 msecs
[ 1358.070007] PM: late suspend of devices complete after 1.126 msecs
[ 1358.072051] PM: noirq suspend of devices complete after 2.040 msecs
[ 1358.072051] Disabling non-boot CPUs ...
[ 1358.073211] CPU1: shutdown
[ 1359.104156] Successfully put all powerdomains to target state
[ 1359.104278] Enabling non-boot CPUs ...
[ 1359.106079] CPU1: Booted secondary processor
[ 1359.106475] CPU1 is up
[ 1359.107482] PM: noirq resume of devices complete after 1.004 msecs
[ 1359.108886] PM: early resume of devices complete after 0.761 msecs
[ 1360.414794] PM: resume of devices complete after 1305.836 msecs
[ 1360.478668] Restarting tasks ... done.
# # grep -i uart /proc/interrupts
106: 3511 0 GIC OMAP UART2
arch/arm/plat-omap/include/plat/omap-serial.h | 1 +
drivers/tty/serial/omap-serial.c | 16 +++++++++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
@@ -1305,7 +1310,6 @@ static int serial_omap_probe(struct platform_device *pdev)pm_runtime_set_autosuspend_delay(&pdev->dev,omap_up_info->autosuspend_timeout);-pm_runtime_irq_safe(&pdev->dev);pm_runtime_enable(&pdev->dev);pm_runtime_get_sync(&pdev->dev);
@@ -1416,6 +1420,9 @@ static int serial_omap_runtime_suspend(struct device *dev)if(!up)return-EINVAL;+if(up->irq_pending)+return-EBUSY;+if(!pdata)return0;
@@ -1452,6 +1459,9 @@ static int serial_omap_runtime_resume(struct device *dev)up->latency=up->calc_latency;schedule_work(&up->qos_work);++if(up->irq_pending)+serial_omap_irq(up->port.irq,up);}return0;
let's not apply this extra patch. It actually caused a regression which
was pretty difficult to trigger. I have now dropped it from my series.
Sorry for the noise.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120821/9128353e/attachment.sig>
From: Felipe Balbi <hidden> Date: 2012-08-23 06:30:56
Hi,
On Tue, Aug 21, 2012 at 03:15:58PM +0300, Felipe Balbi wrote:
Hi guys,
here's a series of cleanup patches to the OMAP serial
driver. A later series could be made re-implementing
DMA using the DMA Engine API. Note that for RX DMA
we could be using RX Timeout IRQ as a hint that we better
use PIO instead ;-)
All patches were tested on my pandaboard, but I'd really
like to receive Tested-by on other platforms.
After this goes in, I'll probably try to get UART wakeup
working again and only after that look at DMA.
Changes since v1:
. improved commit log on patch 9/13 (formerly 10/13)
. removed patch 2/13
. added a new patch switching from spin_lock_irqsave() to spin_lock and
spin_unlock_irqrestore to spin_unlock
Retested with my pandaboard, UART continues to work:
# grep -i uart /proc/interrupts
106: 124 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 189 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 255 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 321 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 387 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 453 0 GIC OMAP UART2
# grep -i uart /proc/interrupts
106: 519 0 GIC OMAP UART2
cheers
ps: if anyone knows a better test for UART, let me know.
for convenience of anyone testing, patches are available on my git tree [1] on
branch uart
[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart
Well, it turns out we found a small issue with one of these patches. We
have already fixed the isse. Will re-send the series soon with a few
more patches added. cheers
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120823/36470e8b/attachment-0001.sig>