| Message ID | 20150518164720.GA29261@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [150518 09:48]: > 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. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > 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) Fixes: ca8bb4aefb93 ("serial: 8250: Revert "tty: serial: 8250_core: read only RX if there is something in the FIFO"") Tested-by: Tony Lindgren <tony@atomide.com> Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sebastian, On 05/18/2015 12:47 PM, Sebastian Andrzej Siewior wrote: > 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. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > Peter is this what you had in mind? Yes, thanks. Functionality looks correct; minor comments below. > 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(-) > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index 9289999cb7c6..7fd02a03c1ed 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -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); Please use either serial_out() or serial_port_out() but not both in the same function. > + > + 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); Same here. > > 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"); 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 > + 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 9289999cb7c6..7fd02a03c1ed 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -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;
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. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- 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(-)