serial: 8250_omap: provide complete custom startup & shutdown callbacks
RFC PATCH 1/1 serial: 8250_omap: provide complete custom startup & shutdown callbacks
The currently in-use port->startup and port->shutdown are "okay". The startup part for instance does the tiny omap extra part and invokes serial8250_do_startup() for the remaining pieces. The workflow in serial8250_do_startup() is okay except for the part where UART_RX is read without a check if there is something to read. I tried to workaround it in commit 0aa525d1 ("tty: serial: 8250_core: read only RX if there is something in the FIFO") but then reverted it later in commit ca8bb4aefb9 ("serial: 8250: Revert "tty: serial: 8250_core: read only RX if there is something in the FIFO"").
This is the second attempt to get it to work on older OMAPs without breaking other chips this time :)
Peter Hurley suggested to pull in the few needed lines from serial8250_do_startup() and drop everything else that is not required including makeing it simpler like using just request_irq() instead the chain handler like it is doing now.
So lets try that.
---
Peter is this what you had in mind? If so I will repost it without the RFC. And I think tony would like a stable + fixes tag for it.
drivers/tty/serial/8250/8250_omap.c | 81 ++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 9 deletions(-)
drivers/tty/serial/8250/8250_omap.c +72 -9
@@ -562,12 +562,36 @@ static irqreturn_t omap_wake_irq(int irq, void *dev_id)return IRQ_NONE ; } +#ifdef CONFIG_SERIAL_8250_DMA +static int omap_8250_dma_handle_irq (struct uart_port *port); +#endif + +static irqreturn_t omap8250_irq (int irq,void *dev_id) +{ +struct uart_port *port = dev_id; +struct uart_8250_port *up =up_to_u8250p (port); +unsigned int iir; +int ret; + +#ifdef CONFIG_SERIAL_8250_DMA +if (up->dma) { + ret =omap_8250_dma_handle_irq (port); +return IRQ_RETVAL (ret); + } +#endif + +serial8250_rpm_get (up); + iir =serial_port_in (port,UART_IIR ); + ret =serial8250_handle_irq (port, iir); +serial8250_rpm_put (up); + +return IRQ_RETVAL (ret); +} +static int omap_8250_startup (struct uart_port *port) { -struct uart_8250_port *up = - container_of(port, struct uart_8250_port, port); +struct uart_8250_port *up =up_to_u8250p (port);struct omap8250_priv *priv = port->private_data; -int ret;if (priv->wakeirq) {
@@ -580,10 +604,31 @@ static int omap_8250_startup(struct uart_port *port)pm_runtime_get_sync (port->dev); - ret =serial8250_do_startup (port); -if (ret) + up->mcr =0 ; +serial_out (up,UART_FCR ,UART_FCR_CLEAR_RCVR |UART_FCR_CLEAR_XMIT ); + +serial_port_out (port,UART_LCR ,UART_LCR_WLEN8 ); + + up->lsr_saved_flags =0 ; + up->msr_saved_flags =0 ; + +if (up->dma) { + ret =serial8250_request_dma (up); +if (ret) { +dev_warn_ratelimited (port->dev, +"failed to request DMA\n" ); + up->dma =NULL ; + } + } + + ret =request_irq (port->irq, omap8250_irq,IRQF_SHARED , +dev_name (port->dev), port); +if (ret <0 ) goto err; + up->ier =UART_IER_RLSI |UART_IER_RDI ; +serial_port_out (port,UART_IER , up->ier); +#ifdef CONFIG_PM up->capabilities |=UART_CAP_RPM ;#endif
@@ -610,8 +655,7 @@ static int omap_8250_startup(struct uart_port *port)static void omap_8250_shutdown (struct uart_port *port) { -struct uart_8250_port *up = - container_of(port, struct uart_8250_port, port); +struct uart_8250_port *up =up_to_u8250p (port);struct omap8250_priv *priv = port->private_data;flush_work (&priv->qos_work);
@@ -621,11 +665,24 @@ static void omap_8250_shutdown(struct uart_port *port)pm_runtime_get_sync (port->dev);serial_out (up,UART_OMAP_WER ,0 ); -serial8250_do_shutdown (port); + + up->ier =0 ; +serial_port_out (port,UART_IER ,0 ); + +if (up->dma) +serial8250_release_dma (up); + +/* +* Disable break condition and FIFOs +*/ +if (up->lcr &UART_LCR_SBC ) +serial_port_out (port,UART_LCR , up->lcr & ~UART_LCR_SBC ); +serial_out (up,UART_FCR ,UART_FCR_CLEAR_RCVR |UART_FCR_CLEAR_XMIT );pm_runtime_mark_last_busy (port->dev);pm_runtime_put_autosuspend (port->dev); +free_irq (port->irq, port);if (priv->wakeirq)free_irq (priv->wakeirq, port); }
@@ -974,6 +1031,12 @@ static inline int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir) }#endif +static int omap8250_no_handle_irq (struct uart_port *port) +{ +WARN_ONCE (1 ,"This shouldn't be used\n" ); +return 0 ; +} +static int omap8250_probe (struct platform_device *pdev) {struct resource *regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
@@ -1075,6 +1138,7 @@ static int omap8250_probe(struct platform_device *pdev)pm_runtime_get_sync (&pdev->dev);omap_serial_fill_features_erratas (&up, priv); + up.port.handle_irq = omap8250_no_handle_irq;#ifdef CONFIG_SERIAL_8250_DMA if (pdev->dev.of_node) {/*
@@ -1088,7 +1152,6 @@ static int omap8250_probe(struct platform_device *pdev) ret =of_property_count_strings (pdev->dev.of_node,"dma-names" );if (ret ==2 ) { up.dma = &priv->omap8250_dma; - up.port.handle_irq = omap_8250_dma_handle_irq; priv->omap8250_dma.fn = the_no_dma_filter_fn; priv->omap8250_dma.tx_dma = omap_8250_tx_dma; priv->omap8250_dma.rx_dma = omap_8250_rx_dma;
Signature · 1 line
2.1.4
Quoted 18 lines · Sebastian Andrzej Siewior
The currently in-use
port->startupandport->shutdownare "okay". The startup part for instance does the tiny omap extra part and invokesserial8250_do_startup()for the remaining pieces. The workflow inserial8250_do_startup()is okay except for the part whereUART_RXis read without a check if there is something to read. I tried to workaround it in commit0aa525d1("tty: serial: 8250_core: read only RX if there is something in the FIFO") but then reverted it later in commitca8bb4aefb9("serial: 8250: Revert "tty: serial: 8250_core: read only RX if there is something in the FIFO"").This is the second attempt to get it to work on older OMAPs without breaking other chips this time :)
Peter Hurley suggested to pull in the few needed lines fromserial8250_do_startup()and drop everything else that is not required including makeing it simpler like using justrequest_irq()instead the chain handler like it is doing now.
So lets try that.
Quoted 4 lines
Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
Peter is this what you had in mind? If so I will repost it without the RFC. And I think tony would like a stable + fixes tag for it.
Yes please, this fixes an oops for omaps in v4.1-rc series:
Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa06a000
...
[<c04217e8>] (mem_serial_in) from [<c0425480>] (serial8250_do_startup+0xe4/0x694) [<c0425480>] (serial8250_do_startup) from [<c0427e48>] (omap_8250_startup+0x70/0x144) [<c0427e48>] (omap_8250_startup) from [<c0425a54>] (serial8250_startup+0x24/0x30) [<c0425a54>] (serial8250_startup) from [<c04208e4>] (uart_startup.part.14+0x8c/0x1a0) [<c04208e4>] (uart_startup.part.14) from [<c0420fec>] (uart_open+0xd8/0x134) [<c0420fec>] (uart_open) from [<c0403e50>] (tty_open+0xdc/0x5e0) [<c0403e50>] (tty_open) from [<c018f008>] (chrdev_open+0xac/0x188)
read only RX if there is something in the FIFO"")
Regards,
Tony
Hi Sebastian,
Quoted 18 lines · Sebastian Andrzej Siewior
The currently in-use
port->startupandport->shutdownare "okay". The startup part for instance does the tiny omap extra part and invokesserial8250_do_startup()for the remaining pieces. The workflow inserial8250_do_startup()is okay except for the part whereUART_RXis read without a check if there is something to read. I tried to workaround it in commit0aa525d1("tty: serial: 8250_core: read only RX if there is something in the FIFO") but then reverted it later in commitca8bb4aefb9("serial: 8250: Revert "tty: serial: 8250_core: read only RX if there is something in the FIFO"").This is the second attempt to get it to work on older OMAPs without breaking other chips this time :)
Peter Hurley suggested to pull in the few needed lines fromserial8250_do_startup()and drop everything else that is not required including makeing it simpler like using justrequest_irq()instead the chain handler like it is doing now.
So lets try that.
Quoted 3 lines
Signed-off-by Sebastian Andrzej Siewior <[email protected]>---
Peter is this what you had in mind?
Yes, thanks.
Functionality looks correct; minor comments below.
Quoted 51 lines
If so I will repost it without the
RFC. And I think tony would like a stable + fixes tag for it.drivers/tty/serial/8250/8250_omap.c | 81 ++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 9 deletions(-)drivers/tty/serial/8250/8250_omap.c +27
-3@@ -562,12 +562,36 @@ static irqreturn_t omap_wake_irq(int irq, void *dev_id)return IRQ_NONE ; } +#ifdef CONFIG_SERIAL_8250_DMA +static int omap_8250_dma_handle_irq (struct uart_port *port); +#endif + +static irqreturn_t omap8250_irq (int irq,void *dev_id) +{ +struct uart_port *port = dev_id; +struct uart_8250_port *up =up_to_u8250p (port); +unsigned int iir; +int ret; + +#ifdef CONFIG_SERIAL_8250_DMA +if (up->dma) { + ret =omap_8250_dma_handle_irq (port); +return IRQ_RETVAL (ret); + } +#endif + +serial8250_rpm_get (up); + iir =serial_port_in (port,UART_IIR ); + ret =serial8250_handle_irq (port, iir); +serial8250_rpm_put (up); + +return IRQ_RETVAL (ret); +} +static int omap_8250_startup (struct uart_port *port) { -struct uart_8250_port *up = - container_of(port, struct uart_8250_port, port); +struct uart_8250_port *up =up_to_u8250p (port);struct omap8250_priv *priv = port->private_data; -int ret;if (priv->wakeirq) {
drivers/tty/serial/8250/8250_omap.c static int omap_8250_startup(struct uart_port *port) · line 610
Quoted 9 lines
drivers/tty/serial/8250/8250_omap.c +4
-2@@ -580,10 +604,31 @@ static int omap_8250_startup(struct uart_port *port)pm_runtime_get_sync (port->dev); - ret =serial8250_do_startup (port); -if (ret) + up->mcr =0 ; +serial_out (up,UART_FCR ,UART_FCR_CLEAR_RCVR |UART_FCR_CLEAR_XMIT ); + +serial_port_out (port,UART_LCR ,UART_LCR_WLEN8 );
Please use either serial_out() or serial_port_out() but not both in the same function.
Quoted 42 lines
drivers/tty/serial/8250/8250_omap.c +23
-3+ + up->lsr_saved_flags =0 ; + up->msr_saved_flags =0 ; + +if (up->dma) { + ret =serial8250_request_dma (up); +if (ret) { +dev_warn_ratelimited (port->dev, +"failed to request DMA\n" ); + up->dma =NULL ; + } + } + + ret =request_irq (port->irq, omap8250_irq,IRQF_SHARED , +dev_name (port->dev), port); +if (ret <0 ) goto err; + up->ier =UART_IER_RLSI |UART_IER_RDI ; +serial_port_out (port,UART_IER , up->ier); +#ifdef CONFIG_PM up->capabilities |=UART_CAP_RPM ;#endif @@ -610,8 +655,7 @@ static int omap_8250_startup(struct uart_port *port)static void omap_8250_shutdown (struct uart_port *port) { -struct uart_8250_port *up = - container_of(port, struct uart_8250_port, port); +struct uart_8250_port *up =up_to_u8250p (port);struct omap8250_priv *priv = port->private_data;flush_work (&priv->qos_work);@@ -621,11 +665,24 @@ static void omap_8250_shutdown(struct uart_port *port)pm_runtime_get_sync (port->dev);serial_out (up,UART_OMAP_WER ,0 ); -serial8250_do_shutdown (port); + + up->ier =0 ; +serial_port_out (port,UART_IER ,0 );
drivers/tty/serial/8250/8250_omap.c static void omap_8250_shutdown(struct uart_port *port) · line 680
Quoted 10 lines
drivers/tty/serial/8250/8250_omap.c +10
-0+ +if (up->dma) +serial8250_release_dma (up); + +/* +* Disable break condition and FIFOs +*/ +if (up->lcr &UART_LCR_SBC ) +serial_port_out (port,UART_LCR , up->lcr & ~UART_LCR_SBC ); +serial_out (up,UART_FCR ,UART_FCR_CLEAR_RCVR |UART_FCR_CLEAR_XMIT );
Same here.
Quoted 9 lines
drivers/tty/serial/8250/8250_omap.c +1
-0pm_runtime_mark_last_busy (port->dev);pm_runtime_put_autosuspend (port->dev); +free_irq (port->irq, port);if (priv->wakeirq)free_irq (priv->wakeirq, port); }
drivers/tty/serial/8250/8250_omap.c static inline int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir) · line 1036
Quoted 6 lines
drivers/tty/serial/8250/8250_omap.c +3
-0@@ -974,6 +1031,12 @@ static inline int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir) }#endif +static int omap8250_no_handle_irq (struct uart_port *port) +{ +WARN_ONCE (1 ,"This shouldn't be used\n" );
I think it might be helpful if the message or a comment briefly identified why this should never happen. Something like,
/* IRQ has not been requested but handling irq?? */ WARN_ONCE(1, "Unexpected irq handling before port startup\n");
Regards,
Peter Hurley
Quoted 23 lines
drivers/tty/serial/8250/8250_omap.c +4
-1+return 0 ; +} +static int omap8250_probe (struct platform_device *pdev) {struct resource *regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 );@@ -1075,6 +1138,7 @@ static int omap8250_probe(struct platform_device *pdev)pm_runtime_get_sync (&pdev->dev);omap_serial_fill_features_erratas (&up, priv); + up.port.handle_irq = omap8250_no_handle_irq;#ifdef CONFIG_SERIAL_8250_DMA if (pdev->dev.of_node) {/* @@ -1088,7 +1152,6 @@ static int omap8250_probe(struct platform_device *pdev) ret =of_property_count_strings (pdev->dev.of_node,"dma-names" );if (ret ==2 ) { up.dma = &priv->omap8250_dma; - up.port.handle_irq = omap_8250_dma_handle_irq; priv->omap8250_dma.fn = the_no_dma_filter_fn; priv->omap8250_dma.tx_dma = omap_8250_tx_dma; priv->omap8250_dma.rx_dma = omap_8250_rx_dma;