Re: [PATCH 2/2] gpio: omap: convert to use generic irq handler
* Grygorii Strashko | 2015-12-12 10:30:10 [+0200]: >git://git.ti.com/~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git sucked in, thanks. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] gpio: omap: convert to use generic irq handler
* Grygorii Strashko | 2015-10-15 19:33:43 [+0300]: >Hi Thomas, > >On 10/13/2015 09:33 PM, Thomas Gleixner wrote: >>Grygorii, >> >>On Tue, 13 Oct 2015, Grygorii Strashko wrote: >>>I'd very appreciate for any advice of how to better proceed with your >>>request. >>> - I can try to apply and re-send only patches marked by '*' >>> - I can prepare branch with all above patches >> >>Please provide a branch on top of 4.1.10 which contains the whole >>lot. I have a look at it and decide then how to go from there. > >I've prepared branch linux-4.1.10-ti-gpio: >in g...@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git could you please provide a git URL for that? >based on top of > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > branch : linux-4.1.y > commit : 27f1b7f Linux 4.1.10 Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
* Bjorn Helgaas | 2015-12-04 12:46:19 [-0600]:
>The backtrace might be OK (maybe slightly overkill), but all the
>stack addresses are certainly irrelevant and distracting. We only
>need enough to recognize the problem. I don't think the modules list
>is relevant either.
I would shorten it to the bare minimum. Also the patch description
itself could be truncated to the required bits…
>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
>> index 8c36880..0415192 100644
>> --- a/drivers/pci/host/pci-dra7xx.c
>> +++ b/drivers/pci/host/pci-dra7xx.c
>> @@ -301,8 +301,19 @@ static int __init dra7xx_add_pcie_port(struct
>> dra7xx_pcie *dra7xx,
>> return -EINVAL;
>> }
>>
>> +/*
>> + * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
>> + * On -RT and if kernel is booting with "threadirqs" cmd line parameter
>> + * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
>> + * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
>> + * which, in turn, will be resolved to handle_simple_irq() call.
>> + * The handle_simple_irq() expected to be called with IRQ disabled, as
>> + * result kernle will display warning:
>> + * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
>> + */
…not to mention this piece. d7ce4377494a ("powerpc/fsl_msi: mark the msi
cascade handler IRQF_NO_THREAD") fixes the same bug in arch/ppc so they
bypassed you fixing it.
>> ret = devm_request_irq(&pdev->dev, pp->irq,
>> - dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
>> + dra7xx_pcie_msi_irq_handler,
>> + IRQF_SHARED | IRQF_NO_THREAD,
>> "dra7-pcie-msi", pp);
>
>There's similar code in exynos_add_pcie_port(), imx6_add_pcie_port(),
>and spear13xx_add_pcie_port(). Do they need similar changes? If not,
>why not?
You are right. The request for the handler exynos_pcie_msi_irq_handler(),
imx6_pcie_msi_handler() and spear13xx_pcie_irq_handler() needs same
treatment.
Additionally we have:
arch/mips/pci/msi-octeon.c: if (request_irq(OCTEON_IRQ_PCI_MSI0,
octeon_msi_interrupt0,
arch/sparc/kernel/pci_msi.c:err = request_irq(irq, sparc64_msiq_interrupt,
0,
drivers/pci/host/pci-tegra.c: err = request_irq(msi->irq, tegra_pcie_msi_irq,
0,
drivers/pci/host/pcie-rcar.c: err = devm_request_irq(&pdev->dev, msi->irq1,
rcar_pcie_msi_irq,
drivers/pci/host/pcie-rcar.c: err = devm_request_irq(&pdev->dev, msi->irq2,
rcar_pcie_msi_irq,
drivers/pci/host/pcie-xilinx.c: err = devm_request_irq(dev, port->irq,
xilinx_pcie_intr_handler,
which require the same kind of fix…
>I see your discussion about DRA7 hardware design, but my impression is
>that this problem affects anybody who calls dw_handle_msi_irq() from a
>handler registered with IRQF_SHARED.
… brecause all of them invoke generic_handle_irq() from the requsted
handler. generic_handle_irq grabs raw_locks and this needs to run in
raw-irq context.
IRQF_SHARED could probably go away. The IRQ is mostlikely exclusive
assigned in each SoC for MSI interrupt demux.
>Bjorn
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
On 11/06/2015 08:59 PM, Grygorii Strashko wrote: > Hi Sebastian, Hi Grygorii, > - IRQF_NO_THREAD is the first considered option for such kind of issues. > But: Now in LKML there are ~60 occurrences of IRQF_NO_THREAD - most of > them are used by Arch code. And It's only used by 6 drivers (drivers/*) > [Addendum 2]. > During past year, I've found only two threads related to IRQF_NO_THREAD > and, in both cases, IRQF_NO_THREAD was added for arch specific IRQs which > can't be threaded > (https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-November/122659.html, > https://lkml.org/lkml/2015/4/21/404). That powerpc patch you reference is doing the same thing you are doing here. > - ARM UP system: TI's am437xx SoCs for example. >Here everything is started from /drivers/irqchip/irq-gic.c -> > gic_handle_irq() > > GIC IRQ handler gic_handle_irq() may process more than one IRQ without > leaving HW IRQ mode > (during my experiments I saw up to 6 IRQs processed in one cycle). not only GIC. But then what good does it do if it leaves and returns immediately back to the routine? > As result, It was concluded, that having such current HW/SW and all IRQs > forced threaded [1], > it will be potentially possible to predict system behavior, because > gic_handle_irq() will > do the same things for most of processed IRQs. > But, once there will be chained [2] or IRQF_NO_THREAD [3] IRQs - complete > unpredictability. I would not go as far as "complete unpredictability". What you do (or should do) is testing the system for longer period of time with different behavior in order to estimate the worst case. You can't predict the system anyway since it is way too complex. Just try something that ensures that cyclictest is no longer cache hot and see what happens then. > So, It was selected as goal to have all PPI IRQs (forced) threaded. And if > someone > will require faster IRQ handling - IRQF_NO_THREAD can be always added, but it > will > be custom solution then. > > I'd be appreciated for your comments - if above problem is not a problem. > Good - IRQF_NO_THREAD forever! Yes, we try to avoid IRQF_NO_THREAD under all circumstances. However it is required for low-level arch code. This includes basically everything that is using raw-locks which includes interrupt controller (the "real" ones like GIC or cascading like MSI or GPIO). Here it is simple - you have a cascading MSI-interrupt controller and as such it should be IRQF_NO_THREAD marked. The latency spikes in worst case are not huge as explained earlier: The only thing your cascading controller is allowed to do is to mark interrupt as pending (which is with threaded interrupts just a task wakeup). And this is not a -RT only problem: it is broken in vanilla linux with threaded interrupts as well. Btw: There is an exception to this rule (as always): If you are on a slow/blocking bus then you don't do IRQF_NO_THREAD. A slow bus would be a GPIO controller behind an I2C/SPI controller which also acts as an interrupt controller. Here you use a threaded-handler + handle_nested_irq(). > Thanks. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PCI: dra7xx: mark dra7xx_pcie_msi irq as IRQF_NO_THREAD
On 11/05/2015 07:43 PM, Grygorii Strashko wrote:
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 6589e7e..31460f4 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -302,8 +302,30 @@ static int __init dra7xx_add_pcie_port(struct
> dra7xx_pcie *dra7xx,
> return -EINVAL;
> }
>
> + /*
> + * Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
> + * On -RT and if kernel is booting with "threadirqs" cmd line parameter
> + * the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
> + * in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
> + * which, in turn, will be resolved to handle_simple_irq() call.
> + * The handle_simple_irq() expected to be called with IRQ disabled, as
> + * result kernle will display warning:
> + * "irq XXX handler YYY+0x0/0x14 enabled interrupts".
> + *
> + * Current DRA7 PCIe hw configuration supports 32 interrupts,
> + * which cannot change because it's hardwired in silicon, and can assume
> + * that only a few (most of the time it will be exactly ONE) of those
> + * interrupts are pending at the same time. So, It's sane way to dial
> + * with above issue by marking dra7xx_pcie_msi IRQ as IRQF_NO_THREAD.
> + * if some platform will utilize a lot of MSI IRQs and will suffer
> + * form -RT latencies degradation because of that - IRQF_NO_THREAD can
> + * be removed and "Warning Annihilation" W/A can be applied [1]
> + *
> + * [1] https://lkml.org/lkml/2015/11/2/578
I don't think this novel is required. Also a reference to a patch which
was not accepted is not a smart idea (since people might think they
will improve their -RT latency by applying it without thinking).
Do you have any *real* numbers where you can say this patch does
better/worse than what you suggester earlier? I'm simply asking because
each gpio-irq multiplexing driver does the same thing.
> + */
> ret = devm_request_irq(&pdev->dev, pp->irq,
> -dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
> +dra7xx_pcie_msi_irq_handler,
> +IRQF_SHARED | IRQF_NO_THREAD,
> "dra7-pcie-msi", pp);
> if (ret) {
> dev_err(&pdev->dev, "failed to request irq\n");
>
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 3/3] serial: 8250_omap: try to avoid IER_RDI with DMA
It has been observed on DRA7-evm that the UART triggers the interrupt and
reading IIR says IIR_NO_INT. It seems that we receive data via RX-DMA
but the interrupt is triggered anyway. I have hardly observed it on
AM335x and not in *that* quantity. On DRA7-evm with continuous transfers
at 3MBaud and CPU running at 1.5Ghz it is possible that the IRQ-core will
close the UART interrupt after "some time" with "nobody cared".
I've seen that by not enabling IER_RDI those spurious interrupts are not
triggered. Also it seems that DMA and RDI cause the "timeout interrupt"
which does not allow RX-DMA to be scheduled even if the FIFO reached the
programmed RX threshold. However without RDI we don't get a notification
if we have less than RX threshold bytes in the FIFO.
This is where we have the rx_dma_wd timer. After programming the RX-DMA
transfer wait HZ / 4 or 250ms for it to complete. If it does not
complete in that time span we cancel the DMA transfer and enable RDI.
RDI will trigger an UART interrupt in case we have bytes in the FIFO.
Once we read bytes manually from the FIFO we enable RX-DMA again
(without RDI) with the same 250ms timeout.
One downside with this approach is that latency sensitive protocols that
transfer less than 48 bytes will have to wait 250ms to complete.
Is there maybe a user interface where one could set small or bulk transfers?
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 33 -
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 383f143b6b36..68d03553617b 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -113,6 +113,7 @@ struct omap8250_priv {
struct uart_8250_dma omap8250_dma;
spinlock_t rx_dma_lock;
bool rx_dma_broken;
+ struct timer_list rx_dma_wd;
};
static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -599,6 +600,7 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
return IRQ_RETVAL(ret);
}
+static void omap8250_rx_dma_wd(unsigned long data);
static int omap_8250_startup(struct uart_port *port)
{
struct uart_8250_port *up = up_to_u8250p(port);
@@ -627,6 +629,10 @@ static int omap_8250_startup(struct uart_port *port)
dev_warn_ratelimited(port->dev,
"failed to request DMA\n");
up->dma = NULL;
+ } else {
+ init_timer(&priv->rx_dma_wd);
+ priv->rx_dma_wd.function = omap8250_rx_dma_wd;
+ priv->rx_dma_wd.data = (unsigned long) up;
}
}
@@ -635,7 +641,9 @@ static int omap_8250_startup(struct uart_port *port)
if (ret < 0)
goto err;
- up->ier = UART_IER_RLSI | UART_IER_RDI;
+ up->ier = UART_IER_RLSI;
+ if (!up->dma->rxchan)
+ up->ier |= UART_IER_RDI;
serial_out(up, UART_IER, up->ier);
#ifdef CONFIG_PM
@@ -670,6 +678,8 @@ static void omap_8250_shutdown(struct uart_port *port)
if (up->dma)
up->dma->rx_dma(up, UART_IIR_RX_TIMEOUT);
+ del_timer_sync(&priv->rx_dma_wd);
+
pm_runtime_get_sync(port->dev);
serial_out(up, UART_OMAP_WER, 0);
@@ -846,6 +856,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p,
unsigned int iir)
if (dma->rx_running)
goto out;
+ if (p->ier & UART_IER_RDI)
+ goto out;
+
desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
dma->rx_size, DMA_DEV_TO_MEM,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -864,6 +877,7 @@ static int omap_8250_rx_dma(struct uart_8250_port *p,
unsigned int iir)
dma->rx_size, DMA_FROM_DEVICE);
dma_async_issue_pending(dma->rxchan);
+ mod_timer(&priv->rx_dma_wd, jiffies + HZ / 4);
out:
spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
return err;
@@ -1044,6 +1058,9 @@ static int omap_8250_dma_handle_irq(struct uart_port
*port)
dma_err = omap_8250_rx_dma(up, iir);
if (dma_err) {
status = serial8250_rx_chars(up, status);
+
+ up->ier &= ~UART_IER_RDI;
+ serial_port_out(port, UART_IER, up->ier);
omap_8250_rx_dma(up, 0);
}
}
@@ -1069,6 +1086,20 @@ static int omap_8250_dma_handle_irq(struct uart_port
*port)
return 1;
}
+static void omap8250_rx_dma_wd(unsigned long data)
+{
+ struct uart_8250_port *up = (void *) data;
+ struct uart
[PATCH 1/3] serial: 8250: move rx_running out of the bitfield
From: John Ogness
That bitfield is modified by read + or + write operation. If someone
sets any of the other two bits it might render the lock useless.
While at it, remove other bitfields as well to avoid more such
errors.
Cc: Greg Kroah-Hartman
Signed-off-by: John Ogness
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index c43f74c53cd9..a407757dcecc 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -42,9 +42,9 @@ struct uart_8250_dma {
size_t rx_size;
size_t tx_size;
- unsigned char tx_running:1;
- unsigned char tx_err: 1;
- unsigned char rx_running:1;
+ unsigned char tx_running;
+ unsigned char tx_err;
+ unsigned char rx_running;
};
struct old_serial_port {
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] serial: 8250_omap: check how many bytes were injected
The function tty_insert_flip_string() returns an int and as such it might fail. So the result is that I kindly asked to insert 48 bytes and the function only insterted 32. I have no idea what to do with the remaining 16 so I think dropping them is the only option. I also increase the buf_overrun counter so userpace has a clue that we lost bytes. Cc: Greg Kroah-Hartman Signed-off-by: Sebastian Andrzej Siewior --- drivers/tty/serial/8250/8250_omap.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 2ac63c8bd946..933f7ef2c004 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -738,6 +738,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error) struct dma_tx_state state; int count; unsigned long flags; + int ret; dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr, dma->rx_size, DMA_FROM_DEVICE); @@ -753,8 +754,10 @@ static void __dma_rx_do_complete(struct uart_8250_port *p, bool error) count = dma->rx_size - state.residue; - tty_insert_flip_string(tty_port, dma->rx_buf, count); - p->port.icount.rx += count; + ret = tty_insert_flip_string(tty_port, dma->rx_buf, count); + + p->port.icount.rx += ret; + p->port.icount.buf_overrun += count - ret; unlock: spin_unlock_irqrestore(&priv->rx_dma_lock, flags); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
The 8250-omap driver requires the DMA-engine driver to support the pause
command in order to properly turn off programmed RX transfer before the
driver stars manually reading from the FIFO.
The lacking support of the requirement has been discovered recently. In
order to stay safe here we disable RX-DMA completly on probe.
The rx_dma_broken assignment on probe could be removed once we working
pause function in omap-dma.
Cc:
Signed-off-by: Sebastian Andrzej Siewior
---
v1…v2:
- disable RX-DMA right on boot
drivers/tty/serial/8250/8250_omap.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index d9a37191a1ae..2ac63c8bd946 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -112,6 +112,7 @@ struct omap8250_priv {
struct work_struct qos_work;
struct uart_8250_dma omap8250_dma;
spinlock_t rx_dma_lock;
+ bool rx_dma_broken;
};
static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -773,6 +774,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
struct omap8250_priv*priv = p->port.private_data;
struct uart_8250_dma*dma = p->dma;
unsigned long flags;
+ int ret;
spin_lock_irqsave(&priv->rx_dma_lock, flags);
@@ -781,7 +783,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
return;
}
- dmaengine_pause(dma->rxchan);
+ ret = dmaengine_pause(dma->rxchan);
+ if (WARN_ON_ONCE(ret))
+ priv->rx_dma_broken = true;
spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
@@ -825,6 +829,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p,
unsigned int iir)
break;
}
+ if (priv->rx_dma_broken)
+ return -EINVAL;
+
spin_lock_irqsave(&priv->rx_dma_lock, flags);
if (dma->rx_running)
@@ -1219,6 +1226,11 @@ static int omap8250_probe(struct platform_device *pdev)
if (of_machine_is_compatible("ti,am33xx"))
priv->habit |= OMAP_DMA_TX_KICK;
+ /*
+* pause is currently not supported atleast on omap-sdma
+* and edma on most earlier kernels.
+*/
+ priv->rx_dma_broken = true;
}
}
#endif
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()
On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote: > I think what people need to learn is that an API in the kernel which > returns an int _can_ fail - it returns an int so it _can_ return an > error code. If it _can_ return an error code, there _will_ be > implementations which _do_. > > If you don't check the return code, either your code doesn't care whether > the function was successful or not, or you're playing with fire. This is > a prime example of playing with fire. > > Let's leave the crappy userspace laziness with regard to error checking > to userspace, and keep it out of the kernel. > > Yes, the DMA engine capabilities may not be sufficient to describe every > detail of DMA engines, but that's absolutely no reason to skimp on error > checking. Had there been some kind of error checking at the site, this > problem would have been spotted before the 8250-omap driver was merged. Let me disable RX-DMA in 8250-omap code and push that stable. Then we won't need a special annotation for pause support because it remains off and is currently about one user. I browsed each driver in drivers/dma each one which does support pause supports it and all of them implement it unconditionally (ipu_idmac grabs a mutex first but this is another story). Adding error checking to 8250-omap like I have it in #1 and disabling RX-DMA in case pause fails looks be reasonable since there is nothing else that can be done I guess. Once we have the missing piece in omap-dma the RX-DMA can be enabled in 8250-omap. Does this sound like a plan we can agree on? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
On 08/10/2015 01:54 PM, Peter Ujfalusi wrote: >> diff --git a/drivers/tty/serial/8250/8250_omap.c >> b/drivers/tty/serial/8250/8250_omap.c >> index 0340ee6ba970..07a11e0935e4 100644 >> --- a/drivers/tty/serial/8250/8250_omap.c >> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port >> *p) >> return; >> } >> >> -dmaengine_pause(dma->rxchan); >> +ret = dmaengine_pause(dma->rxchan); >> +if (WARN_ON_ONCE(ret)) >> +priv->rx_dma_broken = true; > > I don't think this is good thing for the stable _and_ for the mainline at the > same time: > in stable the rx DMA should not be allowed since the stable kernels does not > allow pause/resume with omap-dma, so there the rx DMA should be just disabled > for UART. This change will cause regression since it introduce a WARN_ON_ONCE, > which will be printed if the user tries to use non working feature. Okay. We do have pause support in mainline for edma since v4.2-rc1. This driver can use edma or sdma depending on the configuration. But it is not yet released. So you suggest remove RX-DMA support completely from the 8250-omap, mark it stable, and revert that patch once we have it fixed in sdma? > In mainline you will eventually going to have pause/resume support so this > patch will make no sense there. The way this works is that it has to be fixed upstream before it can be backported stable. Also Russell made clear (for a good reason) that the RX problem has to be fixed upstream before he thinks about acking the SDMA patch. So if you prefer to instead remove RX-DMA until it is fixed, I am all yours. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
On 08/08/2015 02:28 AM, Peter Hurley wrote: >> diff --git a/drivers/tty/serial/8250/8250_omap.c >> b/drivers/tty/serial/8250/8250_omap.c >> index 0340ee6ba970..07a11e0935e4 100644 >> --- a/drivers/tty/serial/8250/8250_omap.c >> +++ b/drivers/tty/serial/8250/8250_omap.c >> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port >> *p) >> return; >> } >> >> -dmaengine_pause(dma->rxchan); >> +ret = dmaengine_pause(dma->rxchan); >> +if (WARN_ON_ONCE(ret)) >> +priv->rx_dma_broken = true; > > No offense, Sebastian, but it boggles my mind that anyone could defend this > as solid api design. We're in the middle of an interrupt handler and the > slave dma driver is /just/ telling us now that it doesn't implement this > functionality?!!? This is the best thing I could come up with. But to be fair: This happens once _very_ early in the process and is 100% reproducible. The WARN_ON should trigger user attention. > The dmaengine api has _so much_ setup and none of it contemplates telling the > consumer that critical functionality is missing? Lets say it is a missing feature which was not noticed / required until now. I have the missing functionality in patch #3. I don't see the point in adding a lookup API for this feature which has to be backported stable just to figure out that RX-DMA might not work. Again: the WARN_ON triggers on the first short RX read _or_ on shutdown of the port which is not after hours using the port. > Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that > interface is pointless. > > Rather than losing /critical data/ here, the interrupt handler should just > busy-wait until dmaengine_tx_status() returns DMA_COMPLETE for the rx_cookie. This might not happen at all. At 115200 I *never* encouraged this. Once the FIFO is filled with less than RX-trigger size than the UART sends the time-out interrupt and the DMA *never* completes. Even if the FIFO reaches trigger size later. So you have to abort the DMA transfer and read data manually from the FIFO. That is why the omap enqueues the RX-transfer upfront (while the FIFO is still empty). It happens *sometimes* that the UART sends a timeout interrupt and the DMA transfer just is started and it has been only observed at 3mbaud (but there were no tests 115200 > x > 3mbaud that I know of). Therefor I think this is a fair fix without hiding anything. > Regards, > Peter Hurley Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported
The 8250-omap driver requires the DMA-engine driver to support the pause
command in order to properly turn off programmed RX transfer before the
driver stars manually reading from the FIFO.
The lacking support of the requirement has been discovered recently. In
order to stay safe here we disable support for RX-DMA as soon as we
notice that it does not work. This should happen very early.
If the user does not want to see this backtrace he can either disable
DMA support (completely or RX-only) or backport the required patches for
edma / omap-dma once they hit mainline.
Cc:
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 0340ee6ba970..07a11e0935e4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -112,6 +112,7 @@ struct omap8250_priv {
struct work_struct qos_work;
struct uart_8250_dma omap8250_dma;
spinlock_t rx_dma_lock;
+ bool rx_dma_broken;
};
static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -761,6 +762,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
struct omap8250_priv*priv = p->port.private_data;
struct uart_8250_dma*dma = p->dma;
unsigned long flags;
+ int ret;
spin_lock_irqsave(&priv->rx_dma_lock, flags);
@@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
return;
}
- dmaengine_pause(dma->rxchan);
+ ret = dmaengine_pause(dma->rxchan);
+ if (WARN_ON_ONCE(ret))
+ priv->rx_dma_broken = true;
spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
@@ -813,6 +817,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p,
unsigned int iir)
break;
}
+ if (priv->rx_dma_broken)
+ return -EINVAL;
+
spin_lock_irqsave(&priv->rx_dma_lock, flags);
if (dma->rx_running)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
omap-dma + 8250_omap: fix the dmaengine_pause()
#1 is something that can go stable and disables RX-DMA should it notice that it does not work. #2 adds the anotation as suggest by Russell. #3 adds the missing feature to omap-dma so dmaengine_pause() works. Once this is merged, the warning from #1 disappears. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()
In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.
Signed-off-by: Sebastian Andrzej Siewior
---
include/linux/dmaengine.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan
*chan)
return -ENOSYS;
}
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
{
if (chan->device->device_pause)
return chan->device->device_pause(chan);
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers
This DMA driver is used by 8250-omap on DRA7-evm. There is one
requirement that is to pause a transfer. This is currently used on the RX
side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
but the DMA controller starts the transfer shortly after.
Before we can manually purge the FIFO we need to pause the transfer,
check how many bytes it already received and terminate the transfer
without it making any progress.
>From testing on the TX side it seems that it is possible that we invoke
pause once the transfer has completed which is indicated by the missing
CCR_ENABLE bit but before the interrupt has been noticed. In that case the
interrupt will come even after disabling it.
The AM572x manual says that we have to wait for the CCR_RD_ACTIVE &
CCR_WR_ACTIVE bits to be gone before programming it again here is the
drain loop. Also it looks like without the drain the TX-transfer makes
sometimes progress.
One note: The pause + resume combo is broken because after resume the
the complete transfer will be programmed again. That means the already
transferred bytes (until the pause event) will be sent again. This is
currently not important for my UART user because it does only pause +
terminate.
v2…v3:
- rephrase the comment based on Russell's information / feedback.
v1…v2:
- move the drain loop into omap_dma_drain_chan() instead of having it
twice.
- allow pause only for DMA_DEV_TO_MEM transfers if non-cyclic. Add a
comment why DMA_MEM_TO_DEV not allowed.
- clear pause on terminate_all. Otherwise pause() + terminate_all()
will keep the pause bit set and we can't pause the following
transfer.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/dma/omap-dma.c | 122 +++--
1 file changed, 88 insertions(+), 34 deletions(-)
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 249445c8a4c6..cb217fab472e 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -299,7 +299,30 @@ static void omap_dma_start(struct omap_chan *c, struct
omap_desc *d)
omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
}
-static void omap_dma_stop(struct omap_chan *c)
+static void omap_dma_drain_chan(struct omap_chan *c)
+{
+ int i;
+ uint32_t val;
+
+ /* Wait for sDMA FIFO to drain */
+ for (i = 0; ; i++) {
+ val = omap_dma_chan_read(c, CCR);
+ if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+ break;
+
+ if (i > 100)
+ break;
+
+ udelay(5);
+ }
+
+ if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+ dev_err(c->vc.chan.device->dev,
+ "DMA drain did not complete on lch %d\n",
+ c->dma_ch);
+}
+
+static int omap_dma_stop(struct omap_chan *c)
{
struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
uint32_t val;
@@ -312,7 +335,6 @@ static void omap_dma_stop(struct omap_chan *c)
val = omap_dma_chan_read(c, CCR);
if (od->plat->errata & DMA_ERRATA_i541 && val & CCR_TRIGGER_SRC) {
uint32_t sysconfig;
- unsigned i;
sysconfig = omap_dma_glbl_read(od, OCP_SYSCONFIG);
val = sysconfig & ~DMA_SYSCONFIG_MIDLEMODE_MASK;
@@ -323,27 +345,18 @@ static void omap_dma_stop(struct omap_chan *c)
val &= ~CCR_ENABLE;
omap_dma_chan_write(c, CCR, val);
- /* Wait for sDMA FIFO to drain */
- for (i = 0; ; i++) {
- val = omap_dma_chan_read(c, CCR);
- if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
- break;
-
- if (i > 100)
- break;
-
- udelay(5);
- }
-
- if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
- dev_err(c->vc.chan.device->dev,
- "DMA drain did not complete on lch %d\n",
- c->dma_ch);
+ omap_dma_drain_chan(c);
omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
} else {
+
+ if (!(val & CCR_ENABLE))
+ return -EINVAL;
+
val &= ~CCR_ENABLE;
omap_dma_chan_write(c, CCR, val);
+
+ omap_dma_drain_chan(c);
}
mb();
@@ -358,6 +371,7 @@ static void omap_dma_stop(struct omap_chan *c)
omap_dma_chan_write(c, CLNK_CTRL, val);
}
+ return 0;
}
static void omap_dma_start_sg(struct omap_chan *c, struct omap_desc *d,
@@ -728,6 +742,8 @@ static enum dma_status omap_dma_tx_status(struct dma_chan
*chan,
} else {
txstate->residue = 0;
}
+
Re: [PATCH v2] dma: omap-dma: add support for pause of non-cyclic transfers
* Russell King - ARM Linux | 2015-08-07 17:26:48 [+0100]: >On Fri, Aug 07, 2015 at 05:36:05PM +0200, Sebastian Andrzej Siewior wrote: >> +/* >> + * We do not allow DMA_MEM_TO_DEV transfers to be paused. >> + * According to RMK the OMAP hardware might prefetch bytes from >> + * memory into its FIFO and not send it to the device due to the >> + * pause. The bytes in the FIFO are cleared on pause. It is >> + * unspecified by how many bytes the source address is updated >> + * if at all. > >Would you mind rewording the above. > >Please take the time to read the manuals for the device you are playing >with. It's mostly documented in there. See the OMAP4430 ES2.x TRM, >16.4.18 and 16.4.19. I didn't connect the dots that well. Thank you. … I updated the comment to: /* * We do not allow DMA_MEM_TO_DEV transfers to be paused. * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer: * "When a channel is disabled during a transfer, the channel undergoes * an abort, unless it is hardware-source-synchronized …". * A source-synchronised channel is one where the fetching of data is * under control of the device. In other words, a device-to-memory * transfer. So, a destination-synchronised channel (which would be a * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE * bit is cleared. * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel * aborts immediately after completion of current read/write * transactions and then the FIFO is cleaned up." The term "cleaned up" * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE * are both clear _before_ disabling the channel, otherwise data loss * will occur. * The problem is that if the channel is active, then device activity * can result in DMA activity starting between reading those as both * clear and the write to DMA_CCR to clear the enable bit hitting the * hardware. If the DMA hardware can't drain the data in its FIFO to the * destination, then data loss "might" occur (say if we write to an UART * and the UART is not accepting any further data). */ would that be okay? >Due to this, the OMAP DMA engine driver was submitted with this in the >cover note: > >"For the OMAP DMAengine driver, there's a few short-comings: > >1. pause/resume support is not implemented; it's not clear whether the > OMAP hardware is capable of supporting this sanely." If I google for it, I find it. pause/resume support for cyclic was added later without a note why it is only supported for cyclic. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 06:07 PM, Peter Hurley wrote:
>> If we look at what 8250-dma.c is doing:
>>
>> if (dma->rx_running) {
>> dmaengine_pause(dma->rxchan);
>>
>> It's 8250-dma.c which is silently _ignoring_ the return code, failing
>> to check that the operation it requested worked. Maybe this should be
>> WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
>> message?
>
> Thanks for the suggestion; I'll hold on to that and push it after we add
> the 8250 omap dma pause in mainline.
I have a patch ready with WARN_ON_ONCE() for 8250-omap and 8250-dma.
This warning would trigger on am335x/edma until v4.2-rc1 and omap-dma
based version is open. I could post it if you want me to.
Besides that those two, there are four other drivers ignoring the
return code dmaengine_pause().
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 05:29 PM, Russell King - ARM Linux wrote:
> On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
>> [ + Greg KH ]
>>
>> On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
>>> As it is something that the driver has _not_ supported, you are clearly
>>> adding a feature to an existing driver. It's not a bug fix.
>>>
> If something else has been converted to pause channels and that is causing
> a problem, then _that_ conversion is where the bug lies, not the lack of
> support in the omap-dma.
>>
>> FWIW, the actual bug is the api that silently does nothing.
>
> Incorrect.
>
> static int omap_dma_pause(struct dma_chan *chan)
> {
> struct omap_chan *c = to_omap_dma_chan(chan);
>
> /* Pause/Resume only allowed with cyclic mode */
> if (!c->cyclic)
> return -EINVAL;
>
> Asking for the channel to be paused will return an error code indicating
> that the request failed. That will be propagated back through to the
> return code of dmaengine_pause().
>
> If we look at what 8250-dma.c is doing:
>
> if (dma->rx_running) {
> dmaengine_pause(dma->rxchan);
>
> It's 8250-dma.c which is silently _ignoring_ the return code, failing
> to check that the operation it requested worked. Maybe this should be
> WARN_ON(dmaengine_pause(dma->rxchan)) or at least it should print a
> message?
I think this is what Peter meant by saying "silently does nothing".
> So, I guess that means that older kernels will just have to remain broken -
> all because the basic testing of the original code was never undertaken
> to ensure that basic stuff like reception of characters worked properly.
Hehe. I wouldn't describe testing at 3mbaud as basic. This reads as I
didn't do any kind of testing at all prior submitting the driver. This
was not the case.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] dma: omap-dma: add support for pause of non-cyclic transfers
This DMA driver is used by 8250-omap on DRA7-evm. There is one
requirement that is to pause a transfer. This is currently used on the RX
side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
but the DMA controller starts the transfer shortly after.
Before we can manually purge the FIFO we need to pause the transfer,
check how many bytes it already received and terminate the transfer
without it making any progress.
>From testing on the TX side it seems that it is possible that we invoke
pause once the transfer has completed which is indicated by the missing
CCR_ENABLE bit but before the interrupt has been noticed. In that case the
interrupt will come even after disabling it.
The AM572x manual says that we have to wait for the CCR_RD_ACTIVE &
CCR_WR_ACTIVE bits to be gone before programming it again here is the
drain loop. Also it looks like without the drain the TX-transfer makes
sometimes progress.
One note: The pause + resume combo is broken because after resume the
the complete transfer will be programmed again. That means the already
transferred bytes (until the pause event) will be sent again. This is
currently not important for my UART user because it does only pause +
terminate.
v1…v2:
- move the drain loop into omap_dma_drain_chan() instead of having it
twice.
- allow pause only for DMA_DEV_TO_MEM transfers if non-cyclic. Add a
comment why DMA_MEM_TO_DEV not allowed.
- clear pause on terminate_all. Otherwise pause() + terminate_all()
will keep the pause bit set and we can't pause the following
transfer.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/dma/omap-dma.c | 107 +
1 file changed, 73 insertions(+), 34 deletions(-)
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 249445c8a4c6..6bbf089d2d7f 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -299,7 +299,30 @@ static void omap_dma_start(struct omap_chan *c, struct
omap_desc *d)
omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
}
-static void omap_dma_stop(struct omap_chan *c)
+static void omap_dma_drain_chan(struct omap_chan *c)
+{
+ int i;
+ uint32_t val;
+
+ /* Wait for sDMA FIFO to drain */
+ for (i = 0; ; i++) {
+ val = omap_dma_chan_read(c, CCR);
+ if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+ break;
+
+ if (i > 100)
+ break;
+
+ udelay(5);
+ }
+
+ if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+ dev_err(c->vc.chan.device->dev,
+ "DMA drain did not complete on lch %d\n",
+ c->dma_ch);
+}
+
+static int omap_dma_stop(struct omap_chan *c)
{
struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
uint32_t val;
@@ -312,7 +335,6 @@ static void omap_dma_stop(struct omap_chan *c)
val = omap_dma_chan_read(c, CCR);
if (od->plat->errata & DMA_ERRATA_i541 && val & CCR_TRIGGER_SRC) {
uint32_t sysconfig;
- unsigned i;
sysconfig = omap_dma_glbl_read(od, OCP_SYSCONFIG);
val = sysconfig & ~DMA_SYSCONFIG_MIDLEMODE_MASK;
@@ -323,27 +345,18 @@ static void omap_dma_stop(struct omap_chan *c)
val &= ~CCR_ENABLE;
omap_dma_chan_write(c, CCR, val);
- /* Wait for sDMA FIFO to drain */
- for (i = 0; ; i++) {
- val = omap_dma_chan_read(c, CCR);
- if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
- break;
-
- if (i > 100)
- break;
-
- udelay(5);
- }
-
- if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
- dev_err(c->vc.chan.device->dev,
- "DMA drain did not complete on lch %d\n",
- c->dma_ch);
+ omap_dma_drain_chan(c);
omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
} else {
+
+ if (!(val & CCR_ENABLE))
+ return -EINVAL;
+
val &= ~CCR_ENABLE;
omap_dma_chan_write(c, CCR, val);
+
+ omap_dma_drain_chan(c);
}
mb();
@@ -358,6 +371,7 @@ static void omap_dma_stop(struct omap_chan *c)
omap_dma_chan_write(c, CLNK_CTRL, val);
}
+ return 0;
}
static void omap_dma_start_sg(struct omap_chan *c, struct omap_desc *d,
@@ -728,6 +742,8 @@ static enum dma_status omap_dma_tx_status(struct dma_chan
*chan,
} else {
txstate->residue = 0;
}
+ if (ret == DMA_IN_PROGRESS && c->paused)
+
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 03:22 PM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 12:36:14PM +0200, Sebastian Andrzej Siewior wrote: >> On 08/07/2015 11:44 AM, Peter Ujfalusi wrote: >>> with a short testing audio did not broke (the only user of pause/resume) >>> Some comments embedded. >>> >>>> Cc: >>> >>> Why stable? This is not fixing any bugs since the PAUSE was not allowed for >>> non cyclic transfers. >> >> Hmmm. The DRA7x was using pause before for UART. I just did not see it >> coming that it was not allowed here. John made a similar change to the >> edma driver and I assumed it went stable but now I see that it was just >> cherry-picked into the ti tree. > > This is *NOT* stable material. > > Pause of these channels is something that omap-dma has *never* supported. > Therefore, it is *not* a regression. What you are doing is *adding* a > feature to the omap-dma driver. That is not stable material in any sense. > Stable is for bug fixes to existing code, not feature enhancements. I didn't consider this as a feature. > If something else has been converted to pause channels and that is causing > a problem, then _that_ conversion is where the bug lies, not the lack of > support in the omap-dma. So we had the 8250-DMA doing pause and all its current users implement it. We have a DMA driver tree which is not used and it not implementing pause (not implementing pause at all). Later we get a combo of 8250-DMA + DMA driver that is broken because the lack of pause and this is noticed a few kernel releases later. The only way of fixing the bug is by implementing the pause feature. Now you are saying that even if I implement this missing feature in a newer kernel I am not allowed to mark it stable despite the fact that it fixes an existing problem in older kernels because it is not a regression. > If it's a result of using some new driver with omap-dma, then the problem > is with whatever introduced that new combination - it's not that omap-dma > is buggy. > > Don't fix bugs in -stable by adding features. That's _no_ way to fix bugs. > > NAK on this feature patch having any kind of stable tag. I already accepted this. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 03:17 PM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 02:35:45PM +0200, Sebastian Andrzej Siewior wrote: >> On 08/07/2015 12:55 PM, Russell King - ARM Linux wrote: >>> On Fri, Aug 07, 2015 at 10:41:57AM +0200, Sebastian Andrzej Siewior wrote: >>>> This DMA driver is used by 8250-omap on DRA7-evm. There is one >>>> requirement that is to pause a transfer. This is currently used on the RX >>>> side. It is possible that the UART HW aborted the RX (UART's RX-timeout) >>>> but the DMA controller starts the transfer shortly after. >>>> Before we can manually purge the FIFO we need to pause the transfer, >>>> check how many bytes it already received and terminate the transfer >>>> without it making any progress. >>>> >>>> >From testing on the TX side it seems that it is possible that we invoke >>>> pause once the transfer has completed which is indicated by the missing >>>> CCR_ENABLE bit but before the interrupt has been noticed. In that case the >>>> interrupt will come even after disabling it. >>> >>> How do you cope with the OMAP DMA hardware clearing its FIFO when you >>> pause it? >> >> I don't > > ... and so you introduce a silent data loss bug into the driver. That's > not very clever. > >> Right now the 820-omap (8250-dma in general, too but they don't use >> this driver) pause only the RX transfer in an error condition. This >> means it is only device-to-mem transfer. I only mentioned the TX >> transfer here since this was easier to test. > > That may be how 8250 works, but 8250 is not everything. You can't ignore > this problem. You have to deal with it - either by not allowing a channel > that would loose data to be paused, or by recovering from that condition. > You're not doing either in your patch. > > Therefore, I have no other option but to NAK your change. Sorry. > > Please fix this. Would it be okay if I only allow pause for RX-transfers? For TX-transfers, I would need to update the start-address so the transfers begins where it stopped. However based on your concern I can't really assume that the position reported by the HW is the correct one. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 01:44 PM, Peter Ujfalusi wrote:
>>>> Cc:
>>>
>>> Why stable? This is not fixing any bugs since the PAUSE was not allowed for
>>> non cyclic transfers.
>>
>> Hmmm. The DRA7x was using pause before for UART. I just did not see it
>> coming that it was not allowed here. John made a similar change to the
>> edma driver and I assumed it went stable but now I see that it was just
>> cherry-picked into the ti tree.
>> If you are not comfortable it being stable material I can drop it.
>
> This change is needed for the UART DMA support if I'm not mistaken and this
> mode is not really supported by older kernels, so having this to implement
> something which is not going to be used in the stable kernels feels somehow
> wrong.
We have the DT pieces since v3.19-rc1. And if I remember correctly I
tested this on am335x-evm and dra7-evm by I the time I posted the
patches. I agree that dra7 support was not the best back then but I am
almost sure that I had vanilla running for testing.
But I don't insist on the stable tag. Consider it dropped.
>>>> Signed-off-by: Sebastian Andrzej Siewior
>>>> ---
>>>> drivers/dma/omap-dma.c | 54
>>>> ++
>>>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
>>>> index 249445c8a4c6..6b8497203caf 100644
>>>> --- a/drivers/dma/omap-dma.c
>>>> +++ b/drivers/dma/omap-dma.c
>>>> @@ -299,7 +299,7 @@ static void omap_dma_start(struct omap_chan *c, struct
>>>> omap_desc *d)
>>>>omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
>>>> }
>>>>
>>>> -static void omap_dma_stop(struct omap_chan *c)
>>>> +static int omap_dma_stop(struct omap_chan *c)
>>>> {
>>>>struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
>>>>uint32_t val;
>>>> @@ -342,8 +342,26 @@ static void omap_dma_stop(struct omap_chan *c)
>>>>
>>>>omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
>>>>} else {
>>>> + int i = 0;
>>>> +
>>>> + if (!(val & CCR_ENABLE))
>>>> + return -EINVAL;
>>>> +
>>>>val &= ~CCR_ENABLE;
>>>>omap_dma_chan_write(c, CCR, val);
>>>> + do {
>>>> + val = omap_dma_chan_read(c, CCR);
>>>> + if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
>>>> + break;
>>>> + if (i > 100)
>>>
>>> if (++i > 100)
>>> break;
>>> to avoid infinite loop?
>>
>> Ah. So I forgot to increment the counter. A few lines above there is
>> the same loop as a workaround for something. This is the same loop. I
>> could merge the loop + warning if you prefer. to have those things in
>> one place. I could also just increment i. Merging the two loops might
>> be better.
>
> The other loop is for handling the ERRATA i541 and the two loops can not be
> merged since the errata handling also require to change in SYSCONFIG register.
yes, but I had in mind is to put the loop into one function so we gain:
+static void omap_dma_drain_chan(struct omap_chan *c)
+{
+ int i;
+ uint32_t val;
+
+ /* Wait for sDMA FIFO to drain */
+ for (i = 0; ; i++) {
+ val = omap_dma_chan_read(c, CCR);
+ if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+ break;
+
+ if (i > 100)
+ break;
+
+ udelay(5);
+ }
+
+ if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+ dev_err(c->vc.chan.device->dev,
+ "DMA drain did not complete on lch %d\n",
+ c->dma_ch);
+}
which is invoked by both parts of the if case (handling the errata not
not) instead of having the same loop twice.
>>>> + break;
>>>> + udelay(5);
>>>> + } while (1);
>>>> +
>>>> + if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
>>>
>>> if (i > 100) ?
>>
>> While that would work, too I think it is more explicit to the reader if
>> you check for the condition that is important to you.
>
> Yeah, I see that the errata handling is doing the same, fine by me.
good.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 12:55 PM, Russell King - ARM Linux wrote: > On Fri, Aug 07, 2015 at 10:41:57AM +0200, Sebastian Andrzej Siewior wrote: >> This DMA driver is used by 8250-omap on DRA7-evm. There is one >> requirement that is to pause a transfer. This is currently used on the RX >> side. It is possible that the UART HW aborted the RX (UART's RX-timeout) >> but the DMA controller starts the transfer shortly after. >> Before we can manually purge the FIFO we need to pause the transfer, >> check how many bytes it already received and terminate the transfer >> without it making any progress. >> >> >From testing on the TX side it seems that it is possible that we invoke >> pause once the transfer has completed which is indicated by the missing >> CCR_ENABLE bit but before the interrupt has been noticed. In that case the >> interrupt will come even after disabling it. > > How do you cope with the OMAP DMA hardware clearing its FIFO when you > pause it? I don't > The problem is that on mem-to-device transfers, the DMA hardware can > prefetch some data from memory into its FIFO before the device wants > the data. If you then disable the channel, the hardware clears the > FIFO. > > It is unspecified whether the hardware updates the source address in > this case, or by how much. So it's pretty hard to undo the prefetching > in software. > > The net result is: data loss. > > This is why I explicitly did not implement pause/resume for non-cyclic > channels. Right now the 820-omap (8250-dma in general, too but they don't use this driver) pause only the RX transfer in an error condition. This means it is only device-to-mem transfer. I only mentioned the TX transfer here since this was easier to test. When I first tested the 8250_omap + DMA on EDMA it seemed that once the UART hardware triggered an time-out interrupt the DMA transfer _never_ started. Based on this I could just cancel the transfer since the RX-DMA and assume zero bytes were transfer. Therefore I ignored the lack of pause support in EDMA. Things were fine until someone used 3mbaud instead 115200. Its been observed that at this speed the UART triggers the time-out interrupt and the DMA transfer just started. Without the support for pause we lost some bytes here and once pause support was added the problem was gone. Now I've been chasing another bug on Dra7 which uses this driver and the lack of pause support is a candidate for my current problem. So at this point, things can't get worse if we pause a transfer and the hardware reported the wrong number of received bytes. The situation can improve if we get the correct number :) The 8250_omap does not pause the TX/RX transfer for the fun of it. As I said, on the TX side we avoid it and on the RX side the transfer is only paused if we receive an error interrupt (= no way out). Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
On 08/07/2015 11:44 AM, Peter Ujfalusi wrote:
> On 08/07/2015 11:41 AM, Sebastian Andrzej Siewior wrote:
>> This DMA driver is used by 8250-omap on DRA7-evm. There is one
>> requirement that is to pause a transfer. This is currently used on the RX
>> side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
>> but the DMA controller starts the transfer shortly after.
>> Before we can manually purge the FIFO we need to pause the transfer,
>> check how many bytes it already received and terminate the transfer
>> without it making any progress.
>>
>> From testing on the TX side it seems that it is possible that we invoke
>> pause once the transfer has completed which is indicated by the missing
>> CCR_ENABLE bit but before the interrupt has been noticed. In that case the
>> interrupt will come even after disabling it.
>>
>> The AM572x manual says that we have to wait for the CCR_RD_ACTIVE &
>> CCR_WR_ACTIVE bits to be gone before programming it again here is the
>> drain loop. Also it looks like without the drain the TX-transfer makes
>> sometimes progress.
>>
>> One note: The pause + resume combo is broken because after resume the
>> the complete transfer will be programmed again. That means the already
>> transferred bytes (until the pause event) will be sent again. This is
>> currently not important for my UART user because it does only pause +
>> terminate.
>
> with a short testing audio did not broke (the only user of pause/resume)
> Some comments embedded.
>
>> Cc:
>
> Why stable? This is not fixing any bugs since the PAUSE was not allowed for
> non cyclic transfers.
Hmmm. The DRA7x was using pause before for UART. I just did not see it
coming that it was not allowed here. John made a similar change to the
edma driver and I assumed it went stable but now I see that it was just
cherry-picked into the ti tree.
If you are not comfortable it being stable material I can drop it.
>> Signed-off-by: Sebastian Andrzej Siewior
>> ---
>> drivers/dma/omap-dma.c | 54
>> ++
>> 1 file changed, 41 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
>> index 249445c8a4c6..6b8497203caf 100644
>> --- a/drivers/dma/omap-dma.c
>> +++ b/drivers/dma/omap-dma.c
>> @@ -299,7 +299,7 @@ static void omap_dma_start(struct omap_chan *c, struct
>> omap_desc *d)
>> omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
>> }
>>
>> -static void omap_dma_stop(struct omap_chan *c)
>> +static int omap_dma_stop(struct omap_chan *c)
>> {
>> struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
>> uint32_t val;
>> @@ -342,8 +342,26 @@ static void omap_dma_stop(struct omap_chan *c)
>>
>> omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
>> } else {
>> +int i = 0;
>> +
>> +if (!(val & CCR_ENABLE))
>> +return -EINVAL;
>> +
>> val &= ~CCR_ENABLE;
>> omap_dma_chan_write(c, CCR, val);
>> +do {
>> +val = omap_dma_chan_read(c, CCR);
>> +if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
>> +break;
>> +if (i > 100)
>
> if (++i > 100)
> break;
> to avoid infinite loop?
Ah. So I forgot to increment the counter. A few lines above there is
the same loop as a workaround for something. This is the same loop. I
could merge the loop + warning if you prefer. to have those things in
one place. I could also just increment i. Merging the two loops might
be better.
>> +break;
>> +udelay(5);
>> +} while (1);
>> +
>> +if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
>
> if (i > 100) ?
While that would work, too I think it is more explicit to the reader if
you check for the condition that is important to you.
>> +dev_err(c->vc.chan.device->dev,
>> +"DMA drain did not complete on lch %d\n",
>> +c->dma_ch);
>> }
>>
>> mb();
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] dma: omap-dma: add support for pause of non-cyclic transfers
This DMA driver is used by 8250-omap on DRA7-evm. There is one
requirement that is to pause a transfer. This is currently used on the RX
side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
but the DMA controller starts the transfer shortly after.
Before we can manually purge the FIFO we need to pause the transfer,
check how many bytes it already received and terminate the transfer
without it making any progress.
>From testing on the TX side it seems that it is possible that we invoke
pause once the transfer has completed which is indicated by the missing
CCR_ENABLE bit but before the interrupt has been noticed. In that case the
interrupt will come even after disabling it.
The AM572x manual says that we have to wait for the CCR_RD_ACTIVE &
CCR_WR_ACTIVE bits to be gone before programming it again here is the
drain loop. Also it looks like without the drain the TX-transfer makes
sometimes progress.
One note: The pause + resume combo is broken because after resume the
the complete transfer will be programmed again. That means the already
transferred bytes (until the pause event) will be sent again. This is
currently not important for my UART user because it does only pause +
terminate.
Cc:
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/dma/omap-dma.c | 54 ++
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 249445c8a4c6..6b8497203caf 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -299,7 +299,7 @@ static void omap_dma_start(struct omap_chan *c, struct
omap_desc *d)
omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
}
-static void omap_dma_stop(struct omap_chan *c)
+static int omap_dma_stop(struct omap_chan *c)
{
struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
uint32_t val;
@@ -342,8 +342,26 @@ static void omap_dma_stop(struct omap_chan *c)
omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
} else {
+ int i = 0;
+
+ if (!(val & CCR_ENABLE))
+ return -EINVAL;
+
val &= ~CCR_ENABLE;
omap_dma_chan_write(c, CCR, val);
+ do {
+ val = omap_dma_chan_read(c, CCR);
+ if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+ break;
+ if (i > 100)
+ break;
+ udelay(5);
+ } while (1);
+
+ if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+ dev_err(c->vc.chan.device->dev,
+ "DMA drain did not complete on lch %d\n",
+ c->dma_ch);
}
mb();
@@ -358,6 +376,7 @@ static void omap_dma_stop(struct omap_chan *c)
omap_dma_chan_write(c, CLNK_CTRL, val);
}
+ return 0;
}
static void omap_dma_start_sg(struct omap_chan *c, struct omap_desc *d,
@@ -728,6 +747,8 @@ static enum dma_status omap_dma_tx_status(struct dma_chan
*chan,
} else {
txstate->residue = 0;
}
+ if (ret == DMA_IN_PROGRESS && c->paused)
+ ret = DMA_PAUSED;
spin_unlock_irqrestore(&c->vc.lock, flags);
return ret;
@@ -1053,28 +1074,33 @@ static int omap_dma_terminate_all(struct dma_chan *chan)
static int omap_dma_pause(struct dma_chan *chan)
{
struct omap_chan *c = to_omap_dma_chan(chan);
+ struct omap_dmadev *od = to_omap_dma_dev(chan->device);
+ unsigned long flags;
+ int ret = -EINVAL;
- /* Pause/Resume only allowed with cyclic mode */
- if (!c->cyclic)
- return -EINVAL;
+ spin_lock_irqsave(&od->irq_lock, flags);
- if (!c->paused) {
- omap_dma_stop(c);
- c->paused = true;
+ if (!c->paused && c->desc) {
+ ret = omap_dma_stop(c);
+ if (!ret)
+ c->paused = true;
}
- return 0;
+ spin_unlock_irqrestore(&od->irq_lock, flags);
+
+ return ret;
}
static int omap_dma_resume(struct dma_chan *chan)
{
struct omap_chan *c = to_omap_dma_chan(chan);
+ struct omap_dmadev *od = to_omap_dma_dev(chan->device);
+ unsigned long flags;
+ int ret = -EINVAL;
- /* Pause/Resume only allowed with cyclic mode */
- if (!c->cyclic)
- return -EINVAL;
+ spin_lock_irqsave(&od->irq_lock, flags);
- if (c->paused) {
+ if (c->paused && c->desc) {
mb();
/* Restore channel link register */
@@ -1082,9 +1108,11 @@ static int omap_dma_resume(struct dma_chan *chan)
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/06/2015 02:31 PM, Sebastian Andrzej Siewior wrote: Hi Peter, >> I'll look at/test this this weekend, ok? > > Sure. I'm currently re-spinning the patches so have everything in > proper pieces. While at it I will take a look at x_char. So now that I actually look at it. If I read this right, we never send the x_char if the TX-DMA never fails to do its job. The comment above uart_send_xchar() says it is high priority. What do you suggest, wait until the transfer completes, send the x_char _or_ pause the transfer send that byte and then send the byte? In both cases we have to wait until for the FIFO-empty interrupt to make sure we don't overrun that TX-FIFO. I *think* waiting until the transfer completes would be simpler but it is not necessarily high priority. >> Regards, >> Peter Hurley Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/06/2015 02:27 PM, Peter Hurley wrote: > Hi Sebastian, Hi Peter, > On 08/04/2015 07:58 AM, Sebastian Andrzej Siewior wrote: >> On 08/03/2015 09:32 PM, Peter Hurley wrote: >> >>>> You mean a function in 8250-dma API which does what I did just here >>>> with the wait_event() and the wake_up in the callback? That way I could >>>> move the termios_wait into the dma struct instead of keeping in the >>>> omap specific part. I am also not sure if OMAP is the only one that may >>>> hang here or the other people just didn't notice it yet. >>> >>> Exactly; and we need to fix DMA wrt x_char anyway. >>> >>> Going back to the dmaengine api, I think something like this might work >>> (as a first approximation): >>> >>> dma_sync_wait(dma->txchan, dma->tx_cookie); >>> dmaengine_pause(dma->txchan); >>> >>> /* remainder of set_termios */ >>> >>> dmaengine_resume(dma->txchan); >>> >>> We could require 8250 core dma to support pause/resume. >> >> I would prefer the waitqueue approach. >> You can't do this while holding the port lock. The lock is taken with >> irqs off so may not see the transfer completing. >> Why do you pause the channel? It may not work without an active >> descriptor and a start without "resume" should work. Also you must >> ensure that DMA's complete callback does not start another transfer if >> there is something queued up (that is why I had the tx_running dance). >> I am not sure if a transfer that is active and then paused will not >> trigger the hang bug if we change the termios in between. > > I'll look at/test this this weekend, ok? Sure. I'm currently re-spinning the patches so have everything in proper pieces. While at it I will take a look at x_char. > Regards, > Peter Hurley > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/03/2015 09:32 PM, Peter Hurley wrote: >> You mean a function in 8250-dma API which does what I did just here >> with the wait_event() and the wake_up in the callback? That way I could >> move the termios_wait into the dma struct instead of keeping in the >> omap specific part. I am also not sure if OMAP is the only one that may >> hang here or the other people just didn't notice it yet. > > Exactly; and we need to fix DMA wrt x_char anyway. > > Going back to the dmaengine api, I think something like this might work > (as a first approximation): > > dma_sync_wait(dma->txchan, dma->tx_cookie); > dmaengine_pause(dma->txchan); > > /* remainder of set_termios */ > > dmaengine_resume(dma->txchan); > > We could require 8250 core dma to support pause/resume. I would prefer the waitqueue approach. You can't do this while holding the port lock. The lock is taken with irqs off so may not see the transfer completing. Why do you pause the channel? It may not work without an active descriptor and a start without "resume" should work. Also you must ensure that DMA's complete callback does not start another transfer if there is something queued up (that is why I had the tx_running dance). I am not sure if a transfer that is active and then paused will not trigger the hang bug if we change the termios in between. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
On 08/03/2015 06:34 PM, Peter Hurley wrote:
> Hi Sebastian,
Hi Peter,
>> struct old_serial_port {
>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>> b/drivers/tty/serial/8250/8250_omap.c
>> index d9a37191a1ae..12249125a218 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -100,9 +100,9 @@ struct omap8250_priv {
>> u8 wer;
>> u8 xon;
>> u8 xoff;
>> -u8 delayed_restore;
>> u16 quot;
>>
>> +wait_queue_head_t termios_wait;
>> bool is_suspending;
>> int wakeirq;
>> int wakeups_enabled;
>> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port
>> *up,
>> static void omap8250_restore_regs(struct uart_8250_port *up)
>> {
>> struct omap8250_priv *priv = up->port.private_data;
>> -struct uart_8250_dma*dma = up->dma;
>> -
>> -if (dma && dma->tx_running) {
>> -/*
>> - * TCSANOW requests the change to occur immediately however if
>> - * we have a TX-DMA operation in progress then it has been
>> - * observed that it might stall and never complete. Therefore we
>> - * delay DMA completes to prevent this hang from happen.
>> - */
>> -priv->delayed_restore = 1;
>> -return;
>> -}
>>
>> serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>> serial_out(up, UART_EFR, UART_EFR_ECB);
>> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port
>> *up)
>> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
>> }
>>
>> +static void omap_8250_dma_tx_complete(void *param);
>> /*
>> * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have
>> have
>> * some differences in how we want to handle flow control.
>> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
>> struct omap8250_priv *priv = up->port.private_data;
>> unsigned char cval = 0;
>> unsigned int baud;
>> +unsigned int complete_dma = 0;
>>
>> switch (termios->c_cflag & CSIZE) {
>> case CS5:
>> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port
>> *port,
>> if (termios->c_iflag & IXANY)
>> up->mcr |= UART_MCR_XONANY;
>> }
>> +
>> +if (up->dma && up->dma->tx_running) {
>> +struct uart_8250_dma*dma = up->dma;
>> +
>> +/*
>> + * TCSANOW requests the change to occur immediately however if
>> + * we have a TX-DMA operation in progress then it has been
>> + * observed that it might stall and never complete. Therefore we
>> + * wait until DMA completes to prevent this hang from happen.
>> + */
>> +
>> +dma->tx_running = 2;
>> +
>> +spin_unlock_irq(&up->port.lock);
>> +wait_event(priv->termios_wait,
>> + dma->tx_running == 3);
>
> Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
> to complete?
Not that I know of. You still need to ensure that once that DMA
completed, nobody triggers another TX transfer before you do what you
planned. This is ensures by the tx_running != 0 and the spin lock.
> Maybe we could wrap that in the 8250 dma api?
You mean a function in 8250-dma API which does what I did just here
with the wait_event() and the wake_up in the callback? That way I could
move the termios_wait into the dma struct instead of keeping in the
omap specific part. I am also not sure if OMAP is the only one that may
hang here or the other people just didn't notice it yet.
> Regards,
> Peter Hurley
>
>> +spin_lock_irq(&up->port.lock);
>> +complete_dma = 1;
>> +}
>> omap8250_restore_regs(up);
>>
>> spin_unlock_irq(&up->port.lock);
>> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
>> /* Don't rewrite B0 */
>> if (tty_termios_baud_rate(termios))
>> tty_termios_encode_baud_rate(termios, baud, baud);
>> +if (complete_dma)
>> +omap_8250_dma_tx_complete(up);
>> }
>>
>> /* same as 8250 except that we may have extra flow bits set in EFR */
>> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>>
>> spin_lock_irqsave(&p->port.lock, flags);
>>
>> +if (dma->tx_running == 2) {
>> +dma->tx_running = 3;
>> +wake_up(&priv->termios_wait);
>> +goto out;
>> +}
>> +
>> dma->tx_running = 0;
>>
>> xmit->tail += dma->tx_size;
>> xmit->tail &= UART_XMIT_SIZE - 1;
>> p->port.icount.tx += dma->tx_size;
>>
>> -if (priv->delayed_restore) {
>> -priv->delayed_restore = 0;
>> -omap8250_restore_regs(p);
>> -}
>> -
>> if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>> uart_write_wakeup(&p->port);
>>
>> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *pa
Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown
* Peter Hurley | 2015-07-30 20:51:10 [-0400]:
>Hi John,
Hi Peter,
>I was never really a fan of the deferred set_termios();
>I think it's more appropriate to wait for tx dma to
>complete in omap_8250_set_termios().
So you want something like this? This was only compile + boot tested
(without triggering the corner case) and I know that 8250.h piece has to
go in a separated patch (as requested in 2/3 of this series). Just checking
if this is what you had in mind.
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index c43f74c53cd9..a407757dcecc 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -42,9 +42,9 @@ struct uart_8250_dma {
size_t rx_size;
size_t tx_size;
- unsigned char tx_running:1;
- unsigned char tx_err: 1;
- unsigned char rx_running:1;
+ unsigned char tx_running;
+ unsigned char tx_err;
+ unsigned char rx_running;
};
struct old_serial_port {
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index d9a37191a1ae..12249125a218 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -100,9 +100,9 @@ struct omap8250_priv {
u8 wer;
u8 xon;
u8 xoff;
- u8 delayed_restore;
u16 quot;
+ wait_queue_head_t termios_wait;
bool is_suspending;
int wakeirq;
int wakeups_enabled;
@@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
static void omap8250_restore_regs(struct uart_8250_port *up)
{
struct omap8250_priv *priv = up->port.private_data;
- struct uart_8250_dma*dma = up->dma;
-
- if (dma && dma->tx_running) {
- /*
-* TCSANOW requests the change to occur immediately however if
-* we have a TX-DMA operation in progress then it has been
-* observed that it might stall and never complete. Therefore we
-* delay DMA completes to prevent this hang from happen.
-*/
- priv->delayed_restore = 1;
- return;
- }
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
serial_out(up, UART_EFR, UART_EFR_ECB);
@@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
up->port.ops->set_mctrl(&up->port, up->port.mctrl);
}
+static void omap_8250_dma_tx_complete(void *param);
/*
* OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
* some differences in how we want to handle flow control.
@@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
struct omap8250_priv *priv = up->port.private_data;
unsigned char cval = 0;
unsigned int baud;
+ unsigned int complete_dma = 0;
switch (termios->c_cflag & CSIZE) {
case CS5:
@@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
if (termios->c_iflag & IXANY)
up->mcr |= UART_MCR_XONANY;
}
+
+ if (up->dma && up->dma->tx_running) {
+ struct uart_8250_dma*dma = up->dma;
+
+ /*
+* TCSANOW requests the change to occur immediately however if
+* we have a TX-DMA operation in progress then it has been
+* observed that it might stall and never complete. Therefore we
+* wait until DMA completes to prevent this hang from happen.
+*/
+
+ dma->tx_running = 2;
+
+ spin_unlock_irq(&up->port.lock);
+ wait_event(priv->termios_wait,
+ dma->tx_running == 3);
+ spin_lock_irq(&up->port.lock);
+ complete_dma = 1;
+ }
omap8250_restore_regs(up);
spin_unlock_irq(&up->port.lock);
@@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);
+ if (complete_dma)
+ omap_8250_dma_tx_complete(up);
}
/* same as 8250 except that we may have extra flow bits set in EFR */
@@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
spin_lock_irqsave(&p->port.lock, flags);
+ if (dma->tx_running == 2) {
+ dma->tx_running = 3;
+ wake_up(&priv->termios_wait);
+ goto out;
+ }
+
dma->tx_running = 0;
xmit->tail += dma->tx_size;
xmit->tail &= UART_XMIT_SIZE - 1;
p->port.icount.tx += dma->tx_size;
- if (priv->delayed_restore) {
- priv->delayed_restore = 0;
- omap8250_restore_regs(p);
- }
-
if (uart_circ_chars_pending(xmit)
Re: [PATCH] gpio: omap: use raw locks for locking
On 07/27/2015 02:50 PM, Linus Walleij wrote: > Patch applied. thanks. > > Now this question appear in my head: > > Is drivers/gpio full of stuff that will not work with the -RT kernel, > and is this a change that should be done mutatis mutandis on > all the GPIO drivers? I described two call paths where you need a rawlock_t. If your gpio driver uses irq_chip_generic then you a rawlock here and things should be fine. In general: If your gpio controller acts as an interrupts controller (that is via chained handler) then you need the raw-locks if you need any locking (if you have a write 1 to mask/unmask/enable/disable register then you probably don't need any locking here at all). If the gpio controller does not act as an interrupt controller than the spinlock_t type should be enough. If your gpio-interrupt controller requests its interrupt via requested_threaded_irq() then it should do handle_nested_irq() and a mutex is probably used for locking. Using request_irq() with "0" flags is kind of broken. It works in IRQ-context and delivers the interrupts with generic_handle_irq() and this one passes it the handler (like handle_edge_irq() / handle_level_irq()) which takes a raw_lock. Now, if you boot the vanilla kernel with threadedirq then the irq-handler runs in threaded context and you can't take a spinlock here anymore. So I think you should use here IRQF_NO_THREAD here (and the raw lock type). I added tglx on Cc: to back up because it is Monday. > Yours, > Linus Walleij > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gpio: omap: use raw locks for locking
This patch converts gpio_bank.lock from a spin_lock into a
raw_spin_lock. The call path is to access this lock is always under a
raw_spin_lock, for instance
- __setup_irq() holds &desc->lock with irq off
+ __irq_set_trigger()
+ omap_gpio_irq_type()
- handle_level_irq() (runs with irqs off therefore raw locks)
+ mask_ack_irq()
+ omap_gpio_mask_irq()
This fixes the obvious backtrace on -RT. However the locking vs context
is not and this is not limited to -RT:
- omap_gpio_irq_type() is called with IRQ off and has an conditional
call to pm_runtime_get_sync() which may sleep. Either it may happen or
it may not happen but pm_runtime_get_sync() should not be called with
irqs off.
- omap_gpio_debounce() is holding the lock with IRQs off.
+ omap2_set_gpio_debounce()
+ clk_prepare_enable()
+ clk_prepare() this one might sleep.
The number of users of gpiod_set_debounce() / gpio_set_debounce()
looks low but still this is not good.
Acked-by: Javier Martinez Canillas
Acked-by: Santosh Shilimkar
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/gpio/gpio-omap.c | 82
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 61a731ff9a07..f56de8de9c55 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,7 +57,7 @@ struct gpio_bank {
u32 saved_datain;
u32 level_mask;
u32 toggle_mask;
- spinlock_t lock;
+ raw_spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
u32 mod_usage;
@@ -498,19 +498,19 @@ static int omap_gpio_irq_type(struct irq_data *d,
unsigned type)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
retval = omap_set_gpio_triggering(bank, offset, type);
if (retval) {
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
goto error;
}
omap_gpio_init_irq(bank, offset);
if (!omap_gpio_is_input(bank, offset)) {
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
retval = -EINVAL;
goto error;
}
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
__irq_set_handler_locked(d->irq, handle_level_irq);
@@ -636,14 +636,14 @@ static int omap_set_gpio_wakeup(struct gpio_bank *bank,
unsigned offset,
return -EINVAL;
}
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
if (enable)
bank->context.wake_en |= gpio_bit;
else
bank->context.wake_en &= ~gpio_bit;
writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -669,10 +669,10 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
omap_enable_gpio_module(bank, offset);
bank->mod_usage |= BIT(offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -682,14 +682,14 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
bank->mod_usage &= ~(BIT(offset));
if (!LINE_USED(bank->irq_usage, offset)) {
omap_set_gpio_direction(bank, offset, 1);
omap_clear_gpio_debounce(bank, offset);
}
omap_disable_gpio_module(bank, offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
@@ -791,7 +791,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data
*d)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
if (!LINE_USED(b
Re: musb-hdrc: "Vbus off, timeout 1100 msec" message does not belong in sysfs
On 07/09/2015 11:46 PM, Pavel Machek wrote:
> On Thu 2015-07-09 23:39:22, Pavel Machek wrote:
>> Hi!
Hi,
>>
>> sysfs should contain one value per file. This one has at least two,
>> with nice english sentence as a bonus.
>>
>> root@n900:/sys/devices/platform/6800.ocp/480ab000.usb_otg_hs/musb-hdrc.0.auto#
>> cat vbus
>> Vbus off, timeout 1100 msec
>>
>> :-(.
>
> Plus, documentation for this is nowhere to be seen:
>
> pavel@amd:/data/l/linux-n900$ grep -ri musb Documentation/ABI/
> pavel@amd:/data/l/linux-n900$
that is a shame indeed. It is part of the driver since TUSB support was
merged via commit 550a7375f ("USB: Add MUSB and TUSB support") about 7
years ago.
The parameter expects the timeout values which is used to poll the port
for mode changes (host/device mode) since it can not be detected on its
own.
Felipe knows best if it is better to document it or remove it. I can't
remember that I needed it on am335x.
> Pavel
>
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On 06/30/2015 08:45 AM, Linus Walleij wrote: > This doesn't apply to the current development tree. it has been pointed out to me. > Can you rebase this patch in Torvalds' HEAD, add Javier's ACK > and resend? I will do it once I'm done roasted :) > Please put me on the To: line, I have no time to read all mail that > I'm only CC on. noted. This might been the problem while it hasn't been applied the first time I posted it. > Yours, > Linus Walleij Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH REPOST] gpio: omap: use raw locks for locking
On 06/30/2015 12:55 PM, Grygorii Strashko wrote:
> Hi Sebastian, All,
Hi Grygorii,
> The problem here is that OMAPs code implemented using PM runtime based
> approach and PM runtime is used everywhere. We don't see warnings form
> other drivers just because their HW IRQ handlers forced to be threaded, but
> that's not the case for OMAP GPIO :(
Only for the case where it is used as an interrupt controller. So the
primary interrupt controller is not using RPM then (or else you would
have the same problem).
> PM runtime for OMAP GPIO is implemented in irq_safe manner and it works for
> non-RT case, but the main question here - How can we proceed for -RT case?
>
> May be you have some thought?
If I remember it properly, you must not sleep but you do so on wakeup.
At least you take spinlocks (spinlocks not raw_spinlocks). One question
is why do you need (or why is it okay) to put a device to sleep (via
RPM) if the device is used as an interrupt controller? From what I
understand, if the GPIO controller is really sleeping you won't receive
any interrupts.
So I think you shouldn't put a GPIO controller to sleep if it is
active. If you avoid this, there should be nothing calling you from
IRQ-context on -RT.
>>From my side what I've tried or thought about:
> 1) OMAP HW IRQ handler can be transformed to threaded IRQ on -RT.
>Can it be acceptable?
It could. In theory. It would work, too. Not sure you really want this.
You lose the cascaded handler that you have now. The obvious change
would be that you see another IRQ used in /proc/interrupts. This is
harmless :) However each time an IRQ (on the GPIO side) arrives you
have a hardirq which is defered into thread context. Then it wakes
another thread (the GPIO-IRQ-handler). So this adds a little of
overhead on your call path. Since you won't have IRQF_NO_THREAD flag on
any user, this should remain the only price you pay.
Also I try not grow the -RT queue. I sent patches upstream where
possible. That means for this I would like to have this addressed
upstream as there is no joy carrying this patch (that means please
don't do a -RT only fix).
> 2) I've tried to use irq_chip->irq_bus_lock/irq_bus_sync_unlock() callbacks.
>It helps a bit, but there are two problems:
>- These callbacks are not expected to fail;
>- PM runtime calls are still present in omap_gpio_irq_handler().
>Hypothetically, they can be removed, but we'd need to ensure that OMAP
> GPIO HW
>is powered and ready to process IRQs all the time while GPIO bank IRQ is
> in use.
>Not trivial thing to do taking into account multi-SoC support, suspend,
> cpuidle :(
What I still don't understand how can a GPIO create an interrupt if it
is sleeping? I know CAN and a few others peripherals can do such
things. Is this also the case for the GPIO?
Or is the problem more that the CPU can't properly enter suspend/
cpuidle if the GPIO bank remains active?
> [code provided just to illustrate idea]
> static void omap_gpio_irq_bus_lock(struct irq_data *data)
> {
>struct gpio_bank *bank = irq_data_get_irq_chip_data(data);
>
>if (!BANK_USED(bank))
>pm_runtime_get_sync(bank->dev);
> }
I don't really like the !BANK_USED(bank) check because it is either in
use or not. Things like that may hide the pm_runtime_get_sync() call and
explode with debug-off.
> I would be appreciated for any comments on this (1 or 2 or ..), thanks.
Another way would be to turn the lock into a raw lock so it won't block
on -RT. However each time you hold the lock in process context you
block a possible -RT task from becoming run-able. Also this could
include larger hacker across the RPM code depending on how this lock
issue is solved.
Either way: the longer you hold the lock the longer you block the -RT
task and the important part is the worst-case scenario.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH REPOST] gpio: omap: use raw locks for locking
This patch converts gpio_bank.lock from a spin_lock into a
raw_spin_lock. The call path is to access this lock is always under a
raw_spin_lock, for instance
- __setup_irq() holds &desc->lock with irq off
+ __irq_set_trigger()
+ omap_gpio_irq_type()
- handle_level_irq() (runs with irqs off therefore raw locks)
+ mask_ack_irq()
+ omap_gpio_mask_irq()
This fixes the obvious backtrace on -RT. However the locking vs context
is not and this is not limited to -RT:
- omap_gpio_irq_type() is called with IRQ off and has an conditional
call to pm_runtime_get_sync() which may sleep. Either it may happen or
it may not happen but pm_runtime_get_sync() should not be called with
irqs off.
- omap_gpio_debounce() is holding the lock with IRQs off.
+ omap2_set_gpio_debounce()
+ clk_prepare_enable()
+ clk_prepare() this one might sleep.
The number of users of gpiod_set_debounce() / gpio_set_debounce()
looks low but still this is not good.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/gpio/gpio-omap.c | 78 +++
1 file changed, 39 insertions(+), 39 deletions(-)
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,7 +57,7 @@ struct gpio_bank {
u32 saved_datain;
u32 level_mask;
u32 toggle_mask;
- spinlock_t lock;
+ raw_spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
u32 mod_usage;
@@ -498,14 +498,14 @@ static int omap_gpio_irq_type(struct irq
(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
return -EINVAL;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
retval = omap_set_gpio_triggering(bank, offset, type);
omap_gpio_init_irq(bank, offset);
if (!omap_gpio_is_input(bank, offset)) {
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return -EINVAL;
}
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
__irq_set_handler_locked(d->irq, handle_level_irq);
@@ -626,14 +626,14 @@ static int omap_set_gpio_wakeup(struct g
return -EINVAL;
}
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
if (enable)
bank->context.wake_en |= gpio_bit;
else
bank->context.wake_en &= ~gpio_bit;
writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -668,7 +668,7 @@ static int omap_gpio_request(struct gpio
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
/* Set trigger to none. You need to enable the desired trigger with
* request_irq() or set_irq_type(). Only do this if the IRQ line has
* not already been requested.
@@ -678,7 +678,7 @@ static int omap_gpio_request(struct gpio
omap_enable_gpio_module(bank, offset);
}
bank->mod_usage |= BIT(offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -688,11 +688,11 @@ static void omap_gpio_free(struct gpio_c
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
bank->mod_usage &= ~(BIT(offset));
omap_disable_gpio_module(bank, offset);
omap_reset_gpio(bank, offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
@@ -794,9 +794,9 @@ static unsigned int omap_gpio_irq_startu
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
omap_gpio_init_irq(bank, offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
omap_gpio_unmask_irq(d);
return 0;
@@ -808,11 +808,11 @@ static void omap_gpio_irq_shutdown(struc
unsigned long flags;
unsigned offset = d->hwirq;
- spin_lock_irqsave(&bank->lock, flags);
+ ra
[PATCH] 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 0aa525d11859 ("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 making it simpler like using just request_irq() instead the
chain handler like it is doing now.
So lets try that.
Fixes: ca8bb4aefb93 ("serial: 8250: Revert "tty: serial: 8250_core:
read only RX if there is something in the FIFO"")
Tested-by: Tony Lindgren
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 82 +
1 file changed, 73 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 928cb7c6..dce1a23706e8 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_out(up, 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_out(up, 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_out(up, UART_IER, 0);
+
+ if (up->dma)
+ serial8250_release_dma(up);
+
+ /*
+* Disable break condition and FIFOs
+*/
+ if (up->lcr & UART_LCR_SBC)
+ serial_out(up, 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,13 @@ static inl
Re: [RFC PATCH] ARM: omap2plus_defconfig: Switch over to using 8250 driver
On 04/12/2015 02:23 AM, Nishanth Menon wrote: >> I think for the moment we should just freeze omap-serial and let >> most of userspace catch up first; a lot of the official and >> unofficial vender support is still stuck in 3.14. By the time, >> 3.19+ is de rigueur we'll hopefully have figured out the ttyS >> sharing and how to migrate without breaking userspace. >> > > How about the folks stuck on older distros like Maemo N900? By the time I was thinking about this, I ended up with this: a) replace ttyOx with ttySx for the kernel console we it does not end up dark and add a warning. This works right now however the warning might been longer but then if one does not read it it might not matter at all. b) add symlink udev rule so ttySx nodes becomes ttyOx one, too a) was mainly for people not aware of the defconfig change who end up with the new driver without any knowledge. b) was mainly for people for people using some kind of "pre-created" image where you are not sure which kernel they have. They still could use the old driver or they might have switched to the new one. This udev rule should make sure there at least a getty on serial after the complete boot. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Hi Shawn, On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote: > We did not add a DT property for it, because there was already enough > info (clock configuration) in DT for kernel to figure it out. Correct. My understanding is whatever can be figured out without DT should be done that way. Is there a way to get this clock-select bit set without enable_fec_anatop_clock() in u-boot? Because this one selects the clock and frequency and also sets the proper bit in gpr1. My question is simply why do we need to do this here as well. > Shawn Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> diff =
> --- arch/arm/mach-imx/mach-imx6q.c
> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>* set bit IOMUXC_GPR1[21]. Or the PTP clock must be from pad
>* (external OSC), and we need to clear the bit.
>*/
> - clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
> IMX6Q_GPR1_ENET_CLK_SEL_PAD;
> gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> if (!IS_ERR(gpr))
Any idea how to do the comparison here? Or should we rely that the bootloader
sets this properly (it managed already to select a frequency)? The phy has no
clock node in current DT's so we can check this.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On 02/16/2015 09:54 AM, Javier Martinez Canillas wrote: > Hello Sebastian, Hello Javier, > Right, those are bugs regardless of PREEMPT_RT or not as you said. > I'll add it to my TODO list, thanks for finding those. Thanks. > Acked-by: Javier Martinez Canillas > > Best regards, > Javier Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] serial: 8250: Revert "tty: serial: 8250_core: read only RX if there is something in the FIFO"
This reverts commit 0aa525d11859c1a4d5b78fdc704148e2ae03ae13.
The conditional RX-FIFO read seems to cause spurious interrupts and we
see just:
|serial8250: too much work for irq29
The previous behaviour was "default" for decades and Marvell's 88f6282 SoC
might not be the only that relies on it. Therefore the Omap fix is
reverted for now.
Fixes: 0aa525d11859 ("tty: serial: 8250_core: read only RX if there is
something in the FIFO")
Reported-By: Nicolas Schichan
Debuged-By: Peter Hurley
Signed-off-by: Sebastian Andrzej Siewior
---
* Russell King - ARM Linux | 2015-02-13 23:15:19 [+]:
>On Fri, Feb 13, 2015 at 07:51:16PM +0100, Sebastian Andrzej Siewior wrote:
>> Something like this maybe?
>
>My personal feeling is that as 0aa525d11859 was wrong, it should be
>reverted and this should be another attempt to fix the problem. In
>other words, there should be two patches, one a revert of the previously
>known bad commit and this one having another go at it.
>
>I feel that would be a better approach, since then we don't end up
>with this change building on a previously know buggy change. It
>would also make the changes to this solution from the previous,
>known-to-work-for-decades code more obvious.
Okay. So here is the revert.
drivers/tty/serial/8250/8250_core.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index e3b9570a1eff..deae122c9c4b 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2138,8 +2138,8 @@ int serial8250_do_startup(struct uart_port *port)
/*
* Clear the interrupt registers.
*/
- if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
- serial_port_in(port, UART_RX);
+ serial_port_in(port, UART_LSR);
+ serial_port_in(port, UART_RX);
serial_port_in(port, UART_IIR);
serial_port_in(port, UART_MSR);
@@ -2300,8 +2300,8 @@ int serial8250_do_startup(struct uart_port *port)
* saved flags to avoid getting false values from polling
* routines or the previous session.
*/
- if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
- serial_port_in(port, UART_RX);
+ serial_port_in(port, UART_LSR);
+ serial_port_in(port, UART_RX);
serial_port_in(port, UART_IIR);
serial_port_in(port, UART_MSR);
up->lsr_saved_flags = 0;
@@ -2394,8 +2394,7 @@ void serial8250_do_shutdown(struct uart_port *port)
* Read data port to reset things, and then unlink from
* the IRQ chain.
*/
- if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
- serial_port_in(port, UART_RX);
+ serial_port_in(port, UART_RX);
serial8250_rpm_put(up);
del_timer_sync(&up->timer);
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
Something like this maybe?
Subject: [PATCH] serial: 8250: skip empty RX-read only on OMAP
The conditional RX-FIFO read seems to cause spurious interrupts and we
see just:
|serial8250: too much work for irq29
The previous behaviour was "default" for decades and Marvell's 88f6282 SoC
might not be the only that relies on it. Therefore this patch moves this
special OMAP3630 behaviour befind a BUG field.
Fixes: 0aa525d11859 ("tty: serial: 8250_core: read only RX if there is
something in the FIFO")
Reported-By: Nicolas Schichan
Debuged-By: Peter Hurley
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_core.c | 10 --
drivers/tty/serial/8250/8250_omap.c | 1 +
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b00836851061..b6899fd69c7e 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -84,6 +84,7 @@ struct serial8250_config {
#define UART_BUG_NOMSR (1 << 2)/* UART has buggy MSR status bits
(Au1x00) */
#define UART_BUG_THRE (1 << 3)/* UART has buggy THRE reassertion */
#define UART_BUG_PARITY(1 << 4)/* UART mishandles parity if
FIFO enabled */
+#define UART_BUG_RXEMPT(1 << 5)/* UART can not read empty FIFO
*/
#define PROBE_RSA (1 << 0)
#define PROBE_ANY (~0)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index e3b9570a1eff..185386178023 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2137,8 +2137,13 @@ int serial8250_do_startup(struct uart_port *port)
/*
* Clear the interrupt registers.
+*
+* This (and later) unsolicied read of the RX FIFO seems to clear the
+* RX timeout condition which otherwise generates spurious interrupt.
+* This behaviour has been observed on Marvell's 88f6282 SoC.
*/
- if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+ if (!(up->bugs & UART_BUG_RXEMPT) ||
+ (serial_port_in(port, UART_LSR) & UART_LSR_DR))
serial_port_in(port, UART_RX);
serial_port_in(port, UART_IIR);
serial_port_in(port, UART_MSR);
@@ -2300,7 +2305,8 @@ int serial8250_do_startup(struct uart_port *port)
* saved flags to avoid getting false values from polling
* routines or the previous session.
*/
- if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+ if (!(up->bugs & UART_BUG_RXEMPT) ||
+ (serial_port_in(port, UART_LSR) & UART_LSR_DR))
serial_port_in(port, UART_RX);
serial_port_in(port, UART_IIR);
serial_port_in(port, UART_MSR);
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index fe6d2e51da09..a7ead2fa6a32 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1021,6 +1021,7 @@ static int omap8250_probe(struct platform_device *pdev)
up.port.fifosize = 64;
up.tx_loadsz = 64;
up.capabilities = UART_CAP_FIFO;
+ up.bugs = UART_BUG_RXEMPT;
#ifdef CONFIG_PM
/*
* Runtime PM is mostly transparent. However to do it right we need to a
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/12/2015 08:55 PM, Peter Hurley wrote: > On 02/12/2015 02:23 PM, Sebastian Andrzej Siewior wrote: >> * Peter Hurley | 2015-02-12 11:32:04 [-0500]: >> >>> That said, I don't think serial8250_do_startup() is really doing that much >>> for OMAP h/w startup; open-coding what omap_8250 really needs is probably >>> < 10 loc. >> >> 10 loc? I have a few more. > > :) > >> serial8250_clear_fifos(), >> serial_link_irq_chain() aren't exported. serial8250_set_mctrl() can >> maybe accessed via uart_ops->set_mctrl(). Maybe I'm not removing the >> obvious not required code but here it looks better to just a BUG flag for >> the Omap. > > Ok. Okay. I will try to post something tomorrow. >> --- a/drivers/tty/serial/8250/8250_omap.c >> +++ b/drivers/tty/serial/8250/8250_omap.c >> + >> +/* >> + * Clear the interrupt registers. >> + */ >> +if (serial_port_in(port, UART_LSR) & UART_LSR_DR) >> +serial_port_in(port, UART_RX); >> +serial_port_in(port, UART_IIR); >> +serial_port_in(port, UART_MSR); >> + >> +retval = serial_link_irq_chain(up); >> +if (retval) >> +goto out; > > omap doesn't really need the legacy irq chain handling; this could just be > request_irq(). > > In the 8250 split I'll be posting soon, all the irq chaining and > polling-via-timeout workarounds stays in the universal/legacy driver so > other 8250 drivers can opt-out. Ah. This sounds interesting. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
* Peter Hurley | 2015-02-12 11:32:04 [-0500]:
>That said, I don't think serial8250_do_startup() is really doing that much
>for OMAP h/w startup; open-coding what omap_8250 really needs is probably
>< 10 loc.
10 loc? I have a few more. serial8250_clear_fifos(),
serial_link_irq_chain() aren't exported. serial8250_set_mctrl() can
maybe accessed via uart_ops->set_mctrl(). Maybe I'm not removing the
obvious not required code but here it looks better to just a BUG flag for
the Omap.
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -557,9 +557,74 @@ static int omap_8250_startup(struct uart_port *port)
pm_runtime_get_sync(port->dev);
- ret = serial8250_do_startup(port);
- if (ret)
- goto err;
+ up->mcr = 0;
+
+ /*
+* Clear the FIFO buffers and disable them.
+* (they will be reenabled in set_termios())
+*/
+ serial8250_clear_fifos(up);
+
+ /*
+* Clear the interrupt registers.
+*/
+ if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+ serial_port_in(port, UART_RX);
+ serial_port_in(port, UART_IIR);
+ serial_port_in(port, UART_MSR);
+
+ retval = serial_link_irq_chain(up);
+ if (retval)
+ goto out;
+
+ /*
+* Now, initialize the UART
+*/
+ serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
+
+ spin_lock_irqsave(&port->lock, flags);
+ /*
+* Most PC uarts need OUT2 raised to enable interrupts.
+*/
+ if (port->irq)
+ up->port.mctrl |= TIOCM_OUT2;
+
+ serial8250_set_mctrl(port, port->mctrl);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ /*
+* Clear the interrupt registers again for luck, and clear the
+* saved flags to avoid getting false values from polling
+* routines or the previous session.
+*/
+ if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+ serial_port_in(port, UART_RX);
+ serial_port_in(port, UART_IIR);
+ serial_port_in(port, UART_MSR);
+ up->lsr_saved_flags = 0;
+ up->msr_saved_flags = 0;
+
+ /*
+* Request DMA channels for both RX and TX.
+*/
+ if (up->dma) {
+ retval = serial8250_request_dma(up);
+ if (retval) {
+ pr_warn_ratelimited("ttyS%d - failed to request DMA\n",
+ serial_index(port));
+ up->dma = NULL;
+ }
+ }
+
+ /*
+* Finally, enable interrupts. Note: Modem status interrupts
+* are set via set_termios(), which will be occurring imminently
+* anyway, so we don't enable them here.
+*/
+ up->ier = UART_IER_RLSI | UART_IER_RDI;
+ serial_port_out(port, UART_IER, up->ier);
+
#ifdef CONFIG_PM
up->capabilities |= UART_CAP_RPM;
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gpio: omap: use raw locks for locking
This patch converts gpio_bank.lock from a spin_lock into a
raw_spin_lock. The call path to access this lock is always under a
raw_spin_lock, for instance
- __setup_irq() holds &desc->lock with irq off
+ __irq_set_trigger()
+ omap_gpio_irq_type()
- handle_level_irq() (runs with irqs off therefore raw locks)
+ mask_ack_irq()
+ omap_gpio_mask_irq()
This fixes the obvious backtrace on -RT. However I noticed two cases
where it looks wrong and this is not limited to -RT:
- omap_gpio_irq_type() is called with IRQ off and has an conditional
call to pm_runtime_get_sync() which may sleep. Either it may happen or
it may not happen but pm_runtime_get_sync() should not be called in an
atomic section.
- omap_gpio_debounce() is holding the lock with IRQs off.
+ omap2_set_gpio_debounce()
+ clk_prepare_enable()
+ clk_prepare() this one might sleep.
The number of users of gpiod_set_debounce() / gpio_set_debounce()
looks low but still this is not good.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/gpio/gpio-omap.c | 78
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f476ae2eb0b3..27e835f4c39d 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,7 +57,7 @@ struct gpio_bank {
u32 saved_datain;
u32 level_mask;
u32 toggle_mask;
- spinlock_t lock;
+ raw_spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
u32 mod_usage;
@@ -515,15 +515,15 @@ static int omap_gpio_irq_type(struct irq_data *d,
unsigned type)
(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
return -EINVAL;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
offset = GPIO_INDEX(bank, gpio);
retval = omap_set_gpio_triggering(bank, offset, type);
omap_gpio_init_irq(bank, gpio, offset);
if (!omap_gpio_is_input(bank, BIT(offset))) {
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return -EINVAL;
}
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
__irq_set_handler_locked(d->irq, handle_level_irq);
@@ -641,14 +641,14 @@ static int omap_set_gpio_wakeup(struct gpio_bank *bank,
int gpio, int enable)
return -EINVAL;
}
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
if (enable)
bank->context.wake_en |= gpio_bit;
else
bank->context.wake_en &= ~gpio_bit;
writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -683,7 +683,7 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
/* Set trigger to none. You need to enable the desired trigger with
* request_irq() or set_irq_type(). Only do this if the IRQ line has
* not already been requested.
@@ -693,7 +693,7 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
omap_enable_gpio_module(bank, offset);
}
bank->mod_usage |= BIT(offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
@@ -703,11 +703,11 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
bank->mod_usage &= ~(BIT(offset));
omap_disable_gpio_module(bank, offset);
omap_reset_gpio(bank, bank->chip.base + offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
/*
* If this is the last gpio to be freed in the bank,
@@ -810,9 +810,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data
*d)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank->dev);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
omap_gpio_init_irq(bank, gpio, offs
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/11/2015 09:42 PM, Peter Hurley wrote: >> Reverting makes sense to me if it has caused a regression. Maybe Sebastian >> can update his patch to do this based on some quirk flag instead? > > That's fine with me. There's a 'bugs' field in struct 8250_uart_port and > UART_BUG_* defines in 8250/8250.h for that purpose. Makes sense. Reading an empty FIFO does not look right. Maybe we should do the bug flag the other way around? But I can do what I am told to so if there is more fallout than just this Marvell UART I could come around with a patch to the bug field for the older OMAP. > Regards, > Peter Hurley Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] tty: serial: 8250_core: read only RX if there is something in the FIFO
On 02/10/2015 12:34 AM, Peter Hurley wrote: > The "too much work" message means serial8250_handle_irq() is returning 0, > ie., not handled. Which in turn means IIR indicates no interrupt is pending > (UART_IIR_NO_INT == 1). The OMAP UART for instance have two possible TX-IRQ handling. The default is to fire the interrupt once there is room for at least one byte in the FIFO. I set the OMAP_UART_SCR_TX_EMPTY bit to get the same behavior as the 8250 driver expects (interrupt while the FIFO is empty). Without this bit set I've seen the message from time to time. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tty: serial: 8250: omap: add ttySx console if the user didn't
This patch invokes add_preferred_console() with ttyS based on ttyO
arguments if the user didn't specify it on its own. This ensures that
the user will see the kernel booting on his serial console in case he
forgot to update the command line.
Signed-off-by: Sebastian Andrzej Siewior
---
While trying to to make a proxy driver like Alan suggested I wasn't sure
how to obtain the ttyS console via "official" API. Then I stumbled upon
update_console_cmdline() and then add_preferred_console().
drivers/tty/serial/8250/8250_omap.c | 40 +
drivers/tty/serial/8250/Kconfig | 19 ++
2 files changed, 59 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 336602eb453e..78401fbc823b 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1248,6 +1248,46 @@ static int omap8250_runtime_resume(struct device *dev)
}
#endif
+#ifdef CONFIG_SERIAL_8250_OMAP_TTYO_FIXUP
+static int __init omap8250_console_fixup(void)
+{
+ char *omap_str;
+ char *options;
+ u8 idx;
+
+ if (strstr(boot_command_line, "console=ttyS"))
+ /* user set a ttyS based name for the console */
+ return 0;
+
+ omap_str = strstr(boot_command_line, "console=ttyO");
+ if (!omap_str)
+ /* user did not set ttyO based console, so we don't care */
+ return 0;
+
+ omap_str += 12;
+ if ('0' <= *omap_str && *omap_str <= '9')
+ idx = *omap_str - '0';
+ else
+ return 0;
+
+ omap_str++;
+ if (omap_str[0] == ',') {
+ omap_str++;
+ options = omap_str;
+ } else {
+ options = NULL;
+ }
+
+ add_preferred_console("ttyS", idx, options);
+ pr_err("WARNING: Your 'console=ttyO%d' has been replaced by 'ttyS%d'\n",
+ idx, idx);
+ pr_err("This ensures that you still see kernel messages. Please\n");
+ pr_err("update your kernel commandline.\n");
+ return 0;
+}
+console_initcall(omap8250_console_fixup);
+#endif
+
static const struct dev_pm_ops omap8250_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(omap8250_suspend, omap8250_resume)
SET_RUNTIME_PM_OPS(omap8250_runtime_suspend,
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 0fcbcd29502f..87adf08b9b6b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -308,6 +308,25 @@ config SERIAL_8250_OMAP
This driver uses ttyS instead of ttyO.
+config SERIAL_8250_OMAP_TTYO_FIXUP
+ bool "Replace ttyO with ttyS"
+ depends on SERIAL_8250_OMAP=y && SERIAL_8250_CONSOLE
+ default y
+ help
+ This option replaces the "console=ttyO" argument with the matching
+ ttyS argument if the user did not specified it on the command line.
+ This ensures that the user can see the kernel output during boot
+ which he wouldn't see otherwise. The getty has still to be configured
+ for ttyS instead of ttyO regardless of this option.
+ This option is intended for people who "automatically" enable this
+ driver without knowing that this driver requires a different console=
+ argument. If you read this, please keep this option disabled and
+ instead update your kernel command line. If you prepare a kernel for a
+ distribution or other kind of larger user base then you probably want
+ to keep this option enabled. Otherwise people might complain about a
+ not booting kernel because the serial console remains silent in case
+ they forgot to update the command line.
+
config SERIAL_8250_FINTEK
tristate "Support for Fintek F81216A LPC to 4 UART"
depends on SERIAL_8250 && PNP
--
2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Touchscreen performance related fixes
On 12/11/2014 09:34 PM, Nicolae Rosia wrote: > Hello, Hi, > Any updates on this series? I don't see it applied in [1] or [2]. I manage to freeze am335x-evm with those patches and I think Vignesh is looking into this. > Regards, > Nicolae Rosia Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: serial: serial-omap: depend on !8250_omap
On 12/01/2014 05:38 PM, Tony Lindgren wrote: > * One Thousand Gnomes [141201 06:11]: >>> Well the nightmare userspace switch from ttyS to ttyO few years ago is >>> something we want to avoid.. I think the best solution would be to make >>> serial-omap.c transparently provide support for ttyO using the new 8250 >>> code so both ttyS and ttyO devices would just work. Otherwise it will >>> be years of "my serial port stopped working" questions again. >> >> Thata a udev problem not a kernel one surely. > > How do you suggest we get people to update their kernel command line > and inittab? Udev may not even be installed. There are three use cases that I can think of right now: - people that enable that new driver via oldconfig I would expect that they read the help message in Kconfig. No worry about them. - people that get a complete system via magic_build_tool (may yocto or whatever) If $TOOL decides to use the new driver, then it should update commandline in bootloader. Those things create usually bootloader + kernel + rootfile system. If the commandline is saved on flash/mmc then it won't be reset from default. However udev should help here. So not a problem either (udev can't fix the kernel boot output but we should see atleast the login console). - people that build omap2plus_defconfig and we switch to the new driver Those people get switched from one driver to the other without knowing. This is what I tried to bring to everyone's attention. The defconfig hasn't been changed yet so it is not problem for next release (yet). I agree that this is a user problem. We agreed not to introduce a console proxy in kernel _or_ hack the command line in kernel (to replace O with S). So I think the problem boils down to educate the user about this change. Making the old driver disappear was one way of getting the user's attention. Another idea would be to introduce a #warning which is also activated by the defconfig and informs the user about the change. Ideally this #warning could be switched off by Kconfig once the user reads & deactivates it. This requires the pay attention to warnings during build. #error would make sure he does but it breaks auto-builds so it is not an option. > Regards, > > Tony > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Touchscreen performance related fixes
On 12/01/2014 10:53 AM, Vignesh R wrote: > Hi Sebastian, Hi Vignesh, > I fixed the issue that was triggering the WARN_ON(). I think it would be > better if you tested these patches, before I posted them on the > mainline. I don't want to clutter the list with too main versions. Here > is the link to the tree with latest changes: awesome, thank you. > > git clone -b tsc-devel git://git.ti.com/kernel/tsc-adc.git > (tsc-devel branch) I will and let you know. > Please test the above tree and let me know the results. > I will post new version(v5) if there are no issues wrt IIO and TSC > working at the same time. Cool. Were you able to get in touch with someone who has a 5-wire touch? > Regards > Vignesh > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: serial: serial-omap: depend on !8250_omap
* Sebastian Andrzej Siewior | 2014-11-26 23:01:46 [+0100]: >Technically speaking this is not required. If both are enabled then the >Maikefile order says that 8250 one wins, the second is never probed. > >If we choose to enable 8250_omap via defconfig then one might get supprised >that his console isn't working anymore since nothing says use ttySx >instead ttyOx. >This patch _tries_ to bring this to the users' attention by not showing >the serial-omap driver once the 8250 one is enabled. So the user might >choose to use the help text which says that this driver (8250_omap) >uses ttySx instead ttyOx. > >Signed-off-by: Sebastian Andrzej Siewior >--- >This is my attempt to warn the defconfig user of the defconfig change >(which did not yet happen). Any suggestions? This is was Uwe Kleine-König suggested off-list: |Currently there are two drivers for the serial device. Until the new one |matured enough to drop the old one, let them conflict each other. They |both handle the same devices, but only one can be responsible for a |single device. There is no technical need, but having two drivers in the |same kernel might result in surprises. I personally don't mind having two drivers enabled since the makefile order is always the same. My main concern is the ttyOx -> ttySx switch after the newer driver is enabled by default via defconfig (and the user does not know it). "make oldconfig" is covered by "default n". Uwe's has a point about "matured enough to drop the old one". It isn't mentioned anywhere that the newer driver supports also DMA. Is this something we want to add to the help text? Could someone that will probably be dealling with possible fallout comment on this? This patch shouldn't go in without an ACK. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tty: serial: serial-omap: depend on !8250_omap
Technically speaking this is not required. If both are enabled then the Maikefile order says that 8250 one wins, the second is never probed. If we choose to enable 8250_omap via defconfig then one might get supprised that his console isn't working anymore since nothing says use ttySx instead ttyOx. This patch _tries_ to bring this to the users' attention by not showing the serial-omap driver once the 8250 one is enabled. So the user might choose to use the help text which says that this driver (8250_omap) uses ttySx instead ttyOx. Signed-off-by: Sebastian Andrzej Siewior --- This is my attempt to warn the defconfig user of the defconfig change (which did not yet happen). Any suggestions? drivers/tty/serial/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index e71a28b4b94e..1b1bdf946fee 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -1125,7 +1125,7 @@ config SERIAL_OF_PLATFORM config SERIAL_OMAP tristate "OMAP serial port support" - depends on ARCH_OMAP2PLUS + depends on ARCH_OMAP2PLUS && !SERIAL_8250_OMAP select SERIAL_CORE help If you have a machine based on an Texas Instruments OMAP CPU you -- 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend
On 11/06/2014 09:35 PM, Tony Lindgren wrote: > The pinctrl change in this patch and the wake-up events are a separate > issue. So this patch can go in and wake-up issue will be fixed once the infrastructure is there. No misunderstanding here? > Tony > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Touchscreen performance related fixes
On 11/24/2014 01:16 PM, Vignesh R wrote: > I have tried running both IIO and TSC at the same time. But I have never > seen WARN_ON() even after running for close to 30 min. Can you send me > the exact script, so that it will be easy to reproduce? Sure thing. - one shell evtest /dev/input/event2 - second shell ./iio-test.sh with: |#!/bin/sh | |while [ 1 = 1 ] |do |cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw |done the kernel config: https://breakpoint.cc/am335x-config > > Regards > Vignesh Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Touchscreen performance related fixes
* Vignesh R | 2014-11-14 10:37:25 [+0530]: >This series of patches fix TSC defects related to lag in touchscreen >performance and cursor jump at touch release. The lag was result of >udelay in TSC interrupt handler. Cursor jump due to false pen-up event. >The patches implement Advisory 1.0.31 in silicon errata of am335x-evm am335x not -evm. The am335x-evm is a board (with its own advisory document) built around the SoC. Just testing the v4. I can use now IIO and touchscren at the same time. back at v1 I reported that it does not work, this has been fixed now. I had it running for a few minutes, now I see one of WARN_ON() beeing triggered (I've cut a few numbers so don't wonder about PID 2 and so on): |dmesg |grep WARNING | wc -l |10 | dmesg |grep WARNING |[306.257995] WARNING: CPU: 0 PID: 97 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[365.469591] WARNING: CPU: 0 PID: 58 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[379.255904] WARNING: CPU: 0 PID: 24 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[426.230505] WARNING: CPU: 0 PID: 35 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[435.654091] WARNING: CPU: 0 PID: 28 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[438.897519] WARNING: CPU: 0 PID: 91 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[525.720193] WARNING: CPU: 0 PID: 88 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[527.644770] WARNING: CPU: 0 PID: 38 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[557.218349] WARNING: CPU: 0 PID: 56 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 |[610.077274] WARNING: CPU: 0 PID: 2 at mfd/ti_am335x_tscadc.c:94 am335x_tsc_se_set_once+0xf8/0x104 The complete trace: |[610.110692] CPU: 0 PID: 4422 Comm: cat Tainted: GW 3.18.0-rc6+ #1745 |[610.118577] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) |[610.126772] [] (show_stack) from [] (warn_slowpath_common+0x68/0x88) |[610.135313] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x1c/0x24) |[610.144586] [] (warn_slowpath_null) from [] (am335x_tsc_se_set_once+0xf8/0x104 [ti_am335x_tscadc]) |[610.155886] [] (am335x_tsc_se_set_once [ti_am335x_tscadc]) from [] (tiadc_read_raw+0xbc/0x190 [ti_am335x_adc]) |[610.168326] [] (tiadc_read_raw [ti_am335x_adc]) from [] (iio_read_channel_info+0x9c/0xa4 [industrialio]) |[610.180191] [] (iio_read_channel_info [industrialio]) from [] (dev_attr_show+0x1c/0x48) |[610.190477] [] (dev_attr_show) from [] (sysfs_kf_seq_show+0x8c/0x110) |[610.199108] [] (sysfs_kf_seq_show) from [] (kernfs_seq_show+0x24/0x28) |[610.207833] [] (kernfs_seq_show) from [] (seq_read+0x1b4/0x47c) |[610.215922] [] (seq_read) from [] (vfs_read+0x8c/0x148) |[610.223269] [] (vfs_read) from [] (SyS_read+0x40/0x8c) |[610.230525] [] (SyS_read) from [] (ret_fast_syscall+0x0/0x30) Could you please look at that one? (I tested it on am335x-evm btw). Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/6] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save
On 11/21/2014 02:10 PM, Richard Cochran wrote: > On the BB white using the LCD4 cape and the shipped debian kernel, the > cursor *does* jump away, but not as often or as far as on the custom > design I was working with. This sounds like the ADC is still sampling while the input data becomes invalid. Usually there is a resistor on the wiper line and the touchscreen manual says how much it should be at least. Could you please check if this is properly installed? > > Thanks, > Richard Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/13] arm: dts: dra7: add DMA properties for UART
On 11/05/2014 08:43 PM, Lennart Sorensen wrote: > I managed to get something dma related on uart3. But it isn't happy: > > [ 95.577401] DMA misaligned error with device 53 > repeated many times. > > I wonder if the dma support isn't quite working for the omap572x yet in > this tree (ti's 3.12.y tree), or maybe it is picky and the driver still > needs a bit of work. misaligned dma? I haven't seen any alignment requirement for SDMA/EDMA and I haven't seen this error on beagle board, am335x-evm or dra7-evm. > I have had no issues on uart7 and 8 without dma. So basically what you are saying is that DMA (no matter which channel) gives you this error and without DMA it works fine? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend
On 11/06/2014 08:15 PM, Nishanth Menon wrote: > sounds good to me *IF* omap8250_enable_wakeup (wakeirq handling) is > the way we want to continue doing things for daisychain? -> Tony, can > you comment? > http://marc.info/?l=linux-omap&m=141222144402707&w=2 > > I wonder if wakeirq explicit handling is valid anymore and something > given the potential race and alternate approach proposed? The wakeirq logic is already in the driver. So if we go for the alternate approach, the pinctrl patch is obsolete? Or does it mean we get rid of the map8250_enable_wakeup() including the second irq we have now (and keep the pinctl change)? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] tty: serial: 8250: omap: Add pinctrl support for suspend
This is mostly an equivalent for the 8250-omap driver based on a patch
of Dave Gerlach for the omap-serial driver (which is not yet merged).
>From his changelog:
|By optionally putting the pins into sleep state in the suspend callback
|we can accomplish two things.
|- minimize current leakage from pins and thus save power,
|- prevent the IP from driving pins output in an uncontrolled manner,
| which may happen if the power domain drops the domain regulator.
The pinctlr change is put before enable_wakeup logic as per Nishanth
Menon (slightly reworded):
|When wakeup is enabled, I/O daisy chain based wakeup is used by
|reconfiguring the padconfig register. However, this gets overriden by
|sleep/wakeup configuration. Therefore we need first to allow pinctrl to
|play with the wakeup bits as needed beyond the sleep configuration.
Cc: Dave Gerlach
Cc: Nishanth Menon
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 57a8b1241b85..1681875f2996 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1156,6 +1156,7 @@ static int omap8250_suspend(struct device *dev)
serial8250_suspend_port(priv->line);
flush_work(&priv->qos_work);
+ pinctrl_pm_select_sleep_state(dev);
if (device_may_wakeup(dev))
omap8250_enable_wakeup(priv, true);
else
@@ -1167,6 +1168,7 @@ static int omap8250_resume(struct device *dev)
{
struct omap8250_priv *priv = dev_get_drvdata(dev);
+ pinctrl_pm_select_default_state(dev);
if (device_may_wakeup(dev))
omap8250_enable_wakeup(priv, false);
--
2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13 v10] omap 8250 based UART + DMA
On 11/06/2014 04:14 AM, Greg KH wrote: > I've now applied the patches here that I could, if I have missed any, > please let me know and resend. Thank you Greg. I pulled your tty-testing and it seems to work on am335x-evm/dra7-evm. Tony, if you could please take the two .dts files then the series would be complete (that one dma patch was applied by Vinod during the merge window). > > thanks, > > greg k-h > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/13] arm: dts: dra7: add DMA properties for UART
On 11/05/2014 05:20 PM, Lennart Sorensen wrote: > On Wed, Nov 05, 2014 at 10:33:15AM -0500, Lennart Sorensen wrote: >> Two systems ran 16 hours each so far with no issues. Pushed 170MB of >> data through the pair of serial ports on one system at 230400. > > The console on uart3 doesn't appear to be using the dma assuming the > values in /sys for the dma controller and bytes transferred mean anything. > It does mention in dmesg that it allocated dma channels for uart3 though. Then it should use it :) > How do you tell if it is using dma? There is omap_8250_tx_dma() and omap_8250_rx_dma(). Both setup callbacks (the rx+tx _complete). Upon successful DMA transfer you should see them invoked with bytes transfered (>0). For RX transfer you need at least trigger bytes in the FIFO within a given time frame (I think it was 46 bytes and the delay may be up to 2 bytes). If you miss this then DMA for RX won't wire and you purge the FIFO manually via "timeout-interrupt" (the callback will be invoked with an error condition and 0 bytes). Assuming this works for you then one should figure out why the counters in /sys are not updated… Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/13] arm: dts: dra7: add DMA properties for UART
On 11/05/2014 02:15 AM, Lennart Sorensen wrote: > Well 4 hours running with multiple reboots (our testsuite reboots every > 30 minutes to test the watchdog). So far it has only lost 70 bytes out > of 40MB of data sent between uart7 and uart8 (and we are pretty sure > the serial test has a small bug that causes a few bytes to be lost right > after a reboot sometimes). > > The reason I am even trying the new driver today is that we were seeing > soft lockups on the CPU whenever the serial ports were being tested, > usually in less than 5 minutes, so 4 hours without a lockup is a good > sign. No idea what might be wrong with the old omap serial driver, but > it seems something is (unless of course we managed to break it somehow > when we tried to solve its habit of dropping characters). Okay. No DMA but the basic part seems to work for you. Thanks for testing. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/13] arm: dts: dra7: add DMA properties for UART
On 11/04/2014 06:02 PM, Lennart Sorensen wrote: > > According to the manual the DMA channels for UART7 to 10 are: > > uart7: 144 145 > uart8: 146 147 > uart9: 148 149 > uart10: 150 151 > > Might as well add those too. We have uart 7 and 8 in use on our board > so I am about to go test it. Mind sharing the manual with me? Mine does not mention DMA channels for UART7-10. If you are able to test 7+8 (and it works) could you please make patch which with for the remaining four devices? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Touchscreen performance related fixes
On 11/04/2014 12:44 PM, Vignesh R wrote: > I ran following commands > $ evtest /dev/input/touchscreen0 & > (with heavy item on touchscreen) > and > $ cat /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en > (in a busy loop) > I tried above experiment on my board but I didn't hit any problem even > after running for close to 30 minutes. I was unable to reproduce failure Just to make sure: You had the touchscreen and iio command running in parallel and you saw evtest output while there was iio operation? > The problem may be in configuring correct charge-delay value. Please run: > $ ts_test > /dev/null > and let me know if pen events are being detected properly. Well I get the touch events but not while busy loop on iio interface is running (I get maybe one event every 5 seconds or so). After all, it is the same HW you have. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Touchscreen performance related fixes
On 10/27/2014 08:02 PM, Griffis, Brad wrote:
> On 10/27/2014 12:34 PM, Sebastian Andrzej Siewior wrote:
>> Do we really need #3 (and then #4)? Given the complexity we have already, is
>> there any benefit by decreasing this value?
>
> I specifically requested we add ti,charge-delay to the device tree because it
> is THE critical value to tune for a given design. Although I think the
> current value of 0xB000 will be suitable for a great many designs, I expect
> that many users will need to adjust this value for their hardware. Details
> such as which touchscreen vendor is being used and how the touchscreen is
> connected (header vs cable) have an effect on what's appropriate here.
Oh. That is one knob I hoped we could avoid since I haven't seen it
before on other TSCs. But okay. Please make sure that there is a
message printed if the default value is used. And lets hope the user
will do something about his.
>> Would someone want to increase it? Can we safely determine a value which
>> works for everyone?
>
> This value represents a hardware delay before checking for the pen-up event.
> So in the scenario where someone is seeing excessive false pen-up events they
> will want to increase this parameter. The downsize of making this larger is
> that it decreases the overall sampling speed of both the touchscreen as well
> as the standalone ADC samples. At one point I tried making it huge, but that
> made the touchscreen overly sluggish because the sampling became too slow.
> So there is a definite trade-off that if you make it too large the
> touchscreen becomes sluggish, and if you make it too small then you may see
> false pen-up events. The optimal value will need to be tuned for a given
> design.
I applied the patches from this series and did the following test on my
am335x-evm: A mug on the touchscreen (to make sure the events are
coming), evtest on the event node to see that the events and loop of
cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw
In the past I was able lock up the TSC/ADC HW that way (see commit
7ca6740cd1 ("mfd: input: iio: ti_amm335x: Rework TSC/ADC
synchronization")) for details.
With this patches applied I don't seen any TSC events once the IIO
interface is (heavily) used. Can this be fixed?
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Touchscreen performance related fixes
On 10/27/2014 12:08 PM, Vignesh R wrote: > This series of patches fix TSC defects related to lag in touchscreen I will try to look this in the next few days. Do we really need #3 (and then #4)? Given the complexity we have already, is there any benefit by decreasing this value? Would someone want to increase it? Can we safely determine a value which works for everyone? Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] tty: serial: omap: Increase max consoles and add check to prevent crash
On 10/22/2014 02:46 PM, Nishanth Menon wrote: > Increase the maximum number of consoles possible to 10 since DRA7 now > has the maximum number of consoles possible. without doing this, for > example, enabling DRA7 UART10 results in internal data structures and > console cannot match up and we endup with a crash as follows: good, Thanks. Acked-by: Sebastian Andrzej Siewior Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tty: serial: omap: increase max consoles to 10
On 10/21/2014 06:23 PM, Nishanth Menon wrote: > > The final solution is to transition off to use 8250 driver and no > dependency on console structures and move away from omap-serial driver, > hence no major cleanups are done for this driver. So the shiny new driver works for you, is this what you are saying? > diff --git a/drivers/tty/serial/omap-serial.c > b/drivers/tty/serial/omap-serial.c > index 18c30ca..4f9cbb6 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -46,7 +46,7 @@ > > #include > > -#define OMAP_MAX_HSUART_PORTS6 > +#define OMAP_MAX_HSUART_PORTS10 > > #define UART_BUILD_REVISION(x, y)(((x) << 8) | (y)) Please also add a check in the probe code that "up->port.line" does not exceed OMAP_MAX_HSUART_PORTS again. So we leave the probe function with an error code instead. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13 v10] omap 8250 based UART + DMA
* Tony Lindgren | 2014-10-02 09:43:39 [-0700]: >Looks good to me. For the patches that do not yet have my acks, please >feel free to add: > >Reviewed-by: Tony Lindgren >Tested-by: Tony Lindgren Thanks. >It's probably best that I queue the .dts changes separately though >to avoid pointless merge conflicts. Yeah. There is no real dependency on them except that you need them at some point :) >Regards, > >Tony Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Frans Klaver | 2014-09-30 10:44:16 [+0200]: >On Mon, Sep 29, 2014 at 12:30:43PM +0200, Frans Klaver wrote: >> On Mon, Sep 29, 2014 at 11:54:40AM +0200, Sebastian Andrzej Siewior wrote: >> > For your "too much work for irq" problem: Could you add trace_printk() >> > in tx/rx dma start/complete, and irq routine? The interresting part is >> > what is the irq routine doing once entered. It might be a condition that >> > is ignored at first and "acked" later while serving another event. Or it >> > is really doing something and this is more or less "legal". >> > >Here's some trace output I get. I hope branches become clear by the >calls they do. > > uart_test-482 [000] .ns.17.860139: __dma_rx_do_complete: error: > 0, uart_bug_dma: 32 > uart_test-482 [000] .ns.17.860141: __dma_rx_do_complete: > rx_dma(p, 0) these two happen outside the IRQ routine for every 48 bytes. … > uart_test-483 [000] dnh.17.861018: serial8250_interrupt: irq 89 > uart_test-483 [000] dnh.17.861026: serial8250_interrupt: 89 e e? Did was the routine invoked 0xe times or this a register? > uart_test-483 [000] .ns.17.861045: __dma_rx_do_complete: error: > 0, uart_bug_dma: 32 > uart_test-483 [000] .ns.17.861047: __dma_rx_do_complete: > rx_dma(p, 0) another 48bytes > uart_test-479 [000] d.H.17.861124: serial8250_interrupt: irq 89 > uart_test-479 [000] d.H.17.861133: serial8250_handle_irq: l1 IIR > cc LSR 61 > uart_test-479 [000] d.H.17.861135: serial8250_handle_irq: > rx_dma(up, iir) > uart_test-479 [000] d.H.17.861139: serial8250_rx_dma: l1, > UART_IIR_RX_TIMEOUT > uart_test-479 [000] d.H.17.861147: __dma_rx_do_complete: error: > 1, uart_bug_dma: 32 > uart_test-479 [000] d.H.17.861150: serial8250_handle_irq: > rx_chars(up, status) > uart_test-479 [000] d.H.17.861190: serial8250_handle_irq: > rx_dma(up, 0) timeout, manual purge. Do you have an idea how many bytes were manually received? > uart_test-479 [000] d.H.17.861205: serial8250_interrupt: 89 e > kworker/0:1-10[000] dnH.17.864949: serial8250_interrupt: irq 89 > >So far so good. We're just entered interrupt where stuff goes awry. The >following pattern repeats over 600 times: > > kworker/0:1-10[000] dnH.17.865198: serial8250_handle_irq: l1 IIR > cc LSR 61 > kworker/0:1-10[000] dnH.17.865254: serial8250_handle_irq: > rx_dma(up, iir) > kworker/0:1-10[000] dnH.17.865333: serial8250_rx_dma: l1, > UART_IIR_RX_TIMEOUT > kworker/0:1-10[000] dnH.17.865626: __dma_rx_do_complete: error: > 1, uart_bug_dma: 32 > kworker/0:1-10[000] dnH.17.865747: serial8250_handle_irq: > rx_chars(up, status) > kworker/0:1-10[000] dnH.17.868797: serial8250_handle_irq: > rx_dma(up, 0) > >ending with: > > kworker/0:1-10[000] dnH.20.027093: serial8250_interrupt: > serial8250: too much work for irq89 > kworker/0:1-10[000] dnH.20.027181: serial8250_interrupt: 89 e > >This clogs the entire system until I disconnect the communication lines. > >So we get an RX timeout. The receiver fifo trigger level was not reached >and 8250_core is left to copy the remaining data. I would expect that if >the trigger level wasn't reached, we wouldn't need to be doing this that >often. On the other hand, could we be trapped reading data from rx >without dma helping us? And how could we resolve this? So if I understand you correct, then you enter serial8250_interrupt() and then you enter multiple times omap_8250_dma_handle_irq() and you always get a TIMEOUT event and fetch byte(s) manualy via serial8250_rx_chars(). And after one iteration UART_IIR_NO_INT is not set and you do it again, right? I have no idea when exactly the timeout-interrupt fires. The manual says: "… the count is reset when there is activity on uarti_rx …" but it does not say how often it increments before the level is reached. It also might be that you have a small gap between two bytes and this high baud rate the gap is considered as a timeout event. Another thing could be that if the we enqueue a RX transfer from the dma_completion callback then we have too many bytes in the FIFO already becahse the callback is invoked from softirq. What happens if we cut the middle man via diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c index 6f80432..21b04bd 100644 --- a/drivers/dma/virt-dma.c +++ b/drivers/dma/virt-dma.c @@ -63,8 +63,9 @@ static void vchan_complete(unsigned long arg) dma_async_tx_callback cb = NULL; void *cb_data = NULL; LIST_HEAD(head); +
[PATCH 10/13] arm: dts: am33xx: add DMA properties for UART
Cc: devicet...@vger.kernel.org
Reviewed-by: Tony Lindgren
Tested-by: Tony Lindgren
Signed-off-by: Sebastian Andrzej Siewior
---
arch/arm/boot/dts/am33xx.dtsi | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 3a0a161342ba..078a760a4a21 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -200,6 +200,8 @@
reg = <0x44e09000 0x2000>;
interrupts = <72>;
status = "disabled";
+ dmas = <&edma 26>, <&edma 27>;
+ dma-names = "tx", "rx";
};
uart1: serial@48022000 {
@@ -209,6 +211,8 @@
reg = <0x48022000 0x2000>;
interrupts = <73>;
status = "disabled";
+ dmas = <&edma 28>, <&edma 29>;
+ dma-names = "tx", "rx";
};
uart2: serial@48024000 {
@@ -218,6 +222,8 @@
reg = <0x48024000 0x2000>;
interrupts = <74>;
status = "disabled";
+ dmas = <&edma 30>, <&edma 31>;
+ dma-names = "tx", "rx";
};
uart3: serial@481a6000 {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/13] tty: serial: 8250: omap: add dma support
This patch adds the required pieces to 8250-OMAP UART driver for DMA
support. The TX burst size is set to 1 so we can send an arbitrary
amount of bytes.
The RX burst is currently set to 48 which means we receive an DMA
interrupt every 48 bytes and have to reprogram everything. Less bytes in
the RX-FIFO mean that no DMA transfer will happen and the UART will send a
RX-timeout _or_ RDI event at which point the FIFO will be manually purged.
There is a workaround for TX-DMA on AM33xx where we put the first byte
into the FIFO to kick start the DMA process. Haven't seen this problem on
OMAP36xx (beagle board xm) or DRA7xx.
On AM375x there is "Usage Note 2.7: UART: Cannot Acknowledge Idle
Requests in Smartidle Mode When Configured for DMA Operations" in the
errata document. This problem persists even after disabling DMA in the
UART and will be addressed in the HWMOD.
v10:
- delay update_registers() from set_termios() until TX-DMA is
done. It has been reported / proved that invoking
update_registers() while TX-DMA is in progress may stall the
DMA operation and it won't finish.
- use the new omap DMA-TX-RX hooks and DMA only interrupt
routine.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 71 +
1 file changed, 71 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 6500547f8fda..57a8b1241b85 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -88,6 +88,7 @@ struct omap8250_priv {
u8 wer;
u8 xon;
u8 xoff;
+ u8 delayed_restore;
u16 quot;
bool is_suspending;
@@ -211,6 +212,18 @@ static void omap8250_update_scr(struct uart_8250_port *up,
static void omap8250_restore_regs(struct uart_8250_port *up)
{
struct omap8250_priv *priv = up->port.private_data;
+ struct uart_8250_dma*dma = up->dma;
+
+ if (dma && dma->tx_running) {
+ /*
+* TCSANOW requests the change to occur immediately however if
+* we have a TX-DMA operation in progress then it has been
+* observed that it might stall and never complete. Therefore we
+* delay DMA completes to prevent this hang from happen.
+*/
+ priv->delayed_restore = 1;
+ return;
+ }
serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
serial_out(up, UART_EFR, UART_EFR_ECB);
@@ -375,6 +388,10 @@ static void omap_8250_set_termios(struct uart_port *port,
priv->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY |
OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
+ if (up->dma)
+ priv->scr |= OMAP_UART_SCR_DMAMODE_1 |
+ OMAP_UART_SCR_DMAMODE_CTL;
+
priv->xon = termios->c_cc[VSTART];
priv->xoff = termios->c_cc[VSTOP];
@@ -554,6 +571,9 @@ static int omap_8250_startup(struct uart_port *port)
priv->wer |= OMAP_UART_TX_WAKEUP_EN;
serial_out(up, UART_OMAP_WER, priv->wer);
+ if (up->dma)
+ up->dma->rx_dma(up, 0);
+
pm_runtime_mark_last_busy(port->dev);
pm_runtime_put_autosuspend(port->dev);
return 0;
@@ -572,6 +592,8 @@ static void omap_8250_shutdown(struct uart_port *port)
struct omap8250_priv *priv = port->private_data;
flush_work(&priv->qos_work);
+ if (up->dma)
+ up->dma->rx_dma(up, UART_IIR_RX_TIMEOUT);
pm_runtime_get_sync(port->dev);
@@ -725,6 +747,7 @@ static void omap_8250_dma_tx_complete(void *param)
struct circ_buf *xmit = &p->port.state->xmit;
unsigned long flags;
boolen_thri = false;
+ struct omap8250_priv*priv = p->port.private_data;
dma_sync_single_for_cpu(dma->txchan->device->dev, dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
@@ -737,6 +760,11 @@ static void omap_8250_dma_tx_complete(void *param)
xmit->tail &= UART_XMIT_SIZE - 1;
p->port.icount.tx += dma->tx_size;
+ if (priv->delayed_restore) {
+ priv->delayed_restore = 0;
+ omap8250_restore_regs(p);
+ }
+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(&p->port);
@@ -909,6 +937,18 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
serial8250_rpm_put(up);
return 1;
}
+
+static bool the_no_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return false;
+}
+
+#else
+
+static inline int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
+{
+ return -EINVAL;
+}
#endif
stat
[PATCH 11/13] arm: dts: dra7: add DMA properties for UART
Cc: devicet...@vger.kernel.org
Reviewed-by: Tony Lindgren
Tested-by: Tony Lindgren
Signed-off-by: Sebastian Andrzej Siewior
---
arch/arm/boot/dts/dra7.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index d678152db4cb..f273e3811f75 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -332,6 +332,8 @@
ti,hwmods = "uart1";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 49>, <&sdma 50>;
+ dma-names = "tx", "rx";
};
uart2: serial@4806c000 {
@@ -341,6 +343,8 @@
ti,hwmods = "uart2";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 51>, <&sdma 52>;
+ dma-names = "tx", "rx";
};
uart3: serial@4802 {
@@ -350,6 +354,8 @@
ti,hwmods = "uart3";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 53>, <&sdma 54>;
+ dma-names = "tx", "rx";
};
uart4: serial@4806e000 {
@@ -359,6 +365,8 @@
ti,hwmods = "uart4";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 55>, <&sdma 56>;
+ dma-names = "tx", "rx";
};
uart5: serial@48066000 {
@@ -368,6 +376,8 @@
ti,hwmods = "uart5";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 63>, <&sdma 64>;
+ dma-names = "tx", "rx";
};
uart6: serial@48068000 {
@@ -377,6 +387,8 @@
ti,hwmods = "uart6";
clock-frequency = <4800>;
status = "disabled";
+ dmas = <&sdma 79>, <&sdma 80>;
+ dma-names = "tx", "rx";
};
uart7: serial@4842 {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/13] tty: serial: 8250: omap: add custom irq handling
We have (or will have) custom DMA callbacks in the omap driver due to
the different behaviour in the RX and TX case. To make this work
we need a few changes in the IRQ handler to invoke the rx_handler again
after the "manual" mode or retry the tx_handler again before falling
back to the manual mode.
Heikki didn't want to see the extra hacks in the generic / default irq
handler and Peter wasn't too happy about an OMAP-only IRQ handler. The
way I planned it is to use this extra IRQ routine only in DMA case. If
Peter dislike this approach then I hope Heikki doesn't block changes in
the default IRQ handler :)
Cc: Heikki Krogerus
Cc: Peter Hurley
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 2 ++
drivers/tty/serial/8250/8250_core.c | 6 ++--
drivers/tty/serial/8250/8250_omap.c | 55 +
3 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 4bb831ab5db0..28097a912c10 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -119,6 +119,8 @@ static inline void serial_dl_write(struct uart_8250_port
*up, int value)
}
struct uart_8250_port *serial8250_get_port(int line);
+void serial8250_rpm_get(struct uart_8250_port *p);
+void serial8250_rpm_put(struct uart_8250_port *p);
#if defined(__alpha__) && !defined(CONFIG_PCI)
/*
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index 93b0799936fd..6a3b4399bf3c 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -541,20 +541,22 @@ void serial8250_clear_and_reinit_fifos(struct
uart_8250_port *p)
}
EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
-static void serial8250_rpm_get(struct uart_8250_port *p)
+void serial8250_rpm_get(struct uart_8250_port *p)
{
if (!(p->capabilities & UART_CAP_RPM))
return;
pm_runtime_get_sync(p->port.dev);
}
+EXPORT_SYMBOL_GPL(serial8250_rpm_get);
-static void serial8250_rpm_put(struct uart_8250_port *p)
+void serial8250_rpm_put(struct uart_8250_port *p)
{
if (!(p->capabilities & UART_CAP_RPM))
return;
pm_runtime_mark_last_busy(p->port.dev);
pm_runtime_put_autosuspend(p->port.dev);
}
+EXPORT_SYMBOL_GPL(serial8250_rpm_put);
/*
* These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 1659858e595a..6500547f8fda 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -854,6 +855,60 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
return ret;
}
+/*
+ * This is mostly serial8250_handle_irq(). We have a slightly different DMA
+ * hoook for RX/TX and need different logic for them in the ISR. Therefore we
+ * use the default routine in the non-DMA case and this one for with DMA.
+ */
+static int omap_8250_dma_handle_irq(struct uart_port *port)
+{
+ struct uart_8250_port *up = up_to_u8250p(port);
+ unsigned char status;
+ unsigned long flags;
+ u8 iir;
+ int dma_err = 0;
+
+ serial8250_rpm_get(up);
+
+ iir = serial_port_in(port, UART_IIR);
+ if (iir & UART_IIR_NO_INT) {
+ serial8250_rpm_put(up);
+ return 0;
+ }
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ status = serial_port_in(port, UART_LSR);
+
+ if (status & (UART_LSR_DR | UART_LSR_BI)) {
+
+ dma_err = omap_8250_rx_dma(up, iir);
+ if (dma_err) {
+ status = serial8250_rx_chars(up, status);
+ omap_8250_rx_dma(up, 0);
+ }
+ }
+ serial8250_modem_status(up);
+ if (status & UART_LSR_THRE && up->dma->tx_err) {
+ if (uart_tx_stopped(&up->port) ||
+ uart_circ_empty(&up->port.state->xmit)) {
+ up->dma->tx_err = 0;
+ serial8250_tx_chars(up);
+ } else {
+ /*
+* try again due to an earlier failer which
+* might have been resolved by now.
+*/
+ dma_err = omap_8250_tx_dma(up);
+ if (dma_err)
+ serial8250_tx_chars(up);
+ }
+ }
+
+ spin_unlock_irqrestore(&port->lock, flags);
+ serial8250_rpm_put(up);
+ return 1;
+}
#endif
static int omap8250_probe(struct platform_device *pdev)
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/13] tty: serial: 8250: make serial8250_console_setup() non _init
if I boot with console=ttyS0 and the omap driver is module I end up with
| console [ttyS0] disabled
| omap8250 44e09000.serial: ttyS0 at MMIO 0x44e09000 (irq = 88, base_baud =
300) is a 8250
| Unable to handle kernel paging request at virtual address c07a9de0
| Modules linked in: 8250_omap(+)
| CPU: 0 PID: 908 Comm: modprobe Not tainted 3.17.0-rc5+ #1593
| PC is at serial8250_console_setup+0x0/0xc8
| LR is at register_console+0x13c/0x3a4
| [] (register_console) from []
(uart_add_one_port+0x3cc/0x420)
| [] (uart_add_one_port) from []
(serial8250_register_8250_port+0x298/0x39c)
| [] (serial8250_register_8250_port) from []
(omap8250_probe+0x218/0x3dc [8250_omap])
| [] (omap8250_probe [8250_omap]) from []
(platform_drv_probe+0x2c/0x5c)
| [] (platform_drv_probe) from []
(driver_probe_device+0x104/0x228)
…
| [] (SyS_init_module) from [] (ret_fast_syscall+0x0/0x30)
| Code: 7823603b f8314620 051b3013 491ed416 (44792204)
because serial8250_console_setup() is already gone.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index a1904628a2a1..159b72471622 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -3237,7 +3237,7 @@ serial8250_console_write(struct console *co, const char
*s, unsigned int count)
serial8250_rpm_put(up);
}
-static int __init serial8250_console_setup(struct console *co, char *options)
+static int serial8250_console_setup(struct console *co, char *options)
{
struct uart_port *port;
int baud = 9600;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/13] tty: serial: Add 8250-core based omap driver
This patch provides a 8250-core based UART driver for the internal OMAP UART. The long term goal is to provide the same functionality as the current OMAP uart driver and DMA support. I tried to merge omap-serial code together with the 8250-core code. There should should be hardly a noticable difference. The trigger levels are different compared to omap-serial: - omap serial TX: Interrupt comes after TX FIFO has room for 16 bytes. TX of 4096 bytes in one go results in 256 interrupts RX: Interrupt comes after there is on byte in the FIFO. RX of 4096 bytes results in 4096 interrupts. - this driver TX: Interrupt comes once the TX FIFO is empty. TX of 4096 bytes results in 65 interrupts. That means there will be gaps on the line while the driver reloads the FIFO. RX: Interrupt comes once there are 48 bytes in the FIFO or less over "longer" time frame. We have 1 / 11520 * 10^3 * 16 => 1.38… ms 1.38ms to react and purge the FIFO on 115200,8N1. Since the other driver fired after each byte it had ~5.47ms time to react. This _may_ cause problems if one relies on no missing bytes and has no flow control. On the other hand we get only 85 interrupts for the same amount of data. It has been only tested as console UART on am335x-evm, dra7-evm and beagle bone. I also did some longer raw-transfers to meassure the load. The device name is ttyS based instead of ttyO. If a ttyO based node name is required please ask udev for it. If both driver are activated (this and omap-serial) then this serial driver will take control over the device due to the link order v9…v10: - Tony noticed that omap3 won't show anything after waking up from core off. In v9 I reworked the register restore and set IER to 0 by accident. This went unnoticed because start_tx usually sets ier (either due to DMA bug or due to TX-complete IRQ). - dropped EFR and SLEEP from capabilities. We do have both but nobody should touch it. We already handle SLEEP ourself. - make the private copy of the registers (like EFR) u8 instead u32 - drop MDR1 & DL[ML] reset in restore registers. Does not look required it is set to the required value later. - update MDR1 & SCR only if changed. - set MDR1 as the last thing. The errata says that we should setup everything before MDR1 set. - avoid div by 0 in omap_8250_get_divisor() if baud rate gets very large (Frans Klaver fixed the same thing omap-serial) - drop "is in early stage" from Kconfig. v8…v9: - less on a file seems to hang the am335x after a while. I believe I introduce this bug a while ago since I can reproduce this prior to v8. Fixed by redoing the omap8250_restore_regs() v7…v8: - redo the register write. There is now one function for that which is used from set_termios() and runtime-resume. - drop PORT_OMAP_16750 and move the setup to the omap file. We have our own set termios function anyway (Heikki Krogerus) - use MEM instead of MEM32. TRM of AM/DM37x says that 32bit access on THR might result in data abort. We only need 32bit access in the errata function which is before we use 8250's read function so it doesn't matter. v4…v7: - change trigger levels after some tests with raw transfers. v3…v4: - drop RS485 support - wire up ->throttle / ->unthrottle v2…v3: - wire up startup & shutdown for wakeup-irq handling. - RS485 handling (well the core does). v1…v2: - added runtime PM. Could somebody could please double check this? - added omap_8250_set_termios() Reviewed-by: Tony Lindgren Tested-by: Tony Lindgren Tested-by: Frans Klaver Signed-off-by: Sebastian Andrzej Siewior --- drivers/tty/serial/8250/8250_omap.c | 914 drivers/tty/serial/8250/Kconfig | 9 + drivers/tty/serial/8250/Makefile| 1 + 3 files changed, 924 insertions(+) create mode 100644 drivers/tty/serial/8250/8250_omap.c diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c new file mode 100644 index ..2f653c48639d --- /dev/null +++ b/drivers/tty/serial/8250/8250_omap.c @@ -0,0 +1,914 @@ +/* + * 8250-core based driver for the OMAP internal UART + * + * based on omap-serial.c, Copyright (C) 2010 Texas Instruments. + * + * Copyright (C) 2014 Sebastian Andrzej Siewior + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "8250.h" + +#define DEFAULT_CLK_SPEED 4800 + +#define UART_ERRATA_i202_MDR1_ACCESS (1 << 0) +#define OMAP_UART_WER_HAS_TX
[PATCH 04/13] tty: serial: 8250_dma: handle error on TX submit
Right now it is possible that serial8250_tx_dma() fails and returns
-EBUSY. The caller (serial8250_start_tx()) will then enable
UART_IER_THRI which will generate an interrupt once the TX FIFO is
empty.
In serial8250_handle_irq() nothing will happen because up->dma is set
and so serial8250_tx_chars() won't be invoked. We end up with plenty of
interrupts and some "too much work for irq" output.
This patch introduces dma_tx_err in struct uart_8250_port to signal that
the last invocation of serial8250_tx_dma() failed so we can fill the TX
FIFO manually. Should the next invocation of serial8250_start_tx()
succeed then the dma_tx_err flag along with the THRI bit is removed and
DMA only usage may continue.
Reviewed-by: Tony Lindgren
Tested-by: Tony Lindgren
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_core.c | 3 ++-
drivers/tty/serial/8250/8250_dma.c | 30 +-
3 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 1bcb4b2141a6..a63d198f8d03 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -41,6 +41,7 @@ struct uart_8250_dma {
size_t tx_size;
unsigned char tx_running:1;
+ unsigned char tx_err: 1;
};
struct old_serial_port {
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index 159b72471622..ea57c87f8528 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1594,7 +1594,8 @@ int serial8250_handle_irq(struct uart_port *port,
unsigned int iir)
status = serial8250_rx_chars(up, status);
}
serial8250_modem_status(up);
- if (!up->dma && (status & UART_LSR_THRE))
+ if ((!up->dma || (up->dma && up->dma->tx_err)) &&
+ (status & UART_LSR_THRE))
serial8250_tx_chars(up);
spin_unlock_irqrestore(&port->lock, flags);
diff --git a/drivers/tty/serial/8250/8250_dma.c
b/drivers/tty/serial/8250/8250_dma.c
index 148ffe4c232f..69e54abb6e71 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -36,8 +36,16 @@ static void __dma_tx_complete(void *param)
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(&p->port);
- if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port))
- serial8250_tx_dma(p);
+ if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port)) {
+ int ret;
+
+ ret = serial8250_tx_dma(p);
+ if (ret) {
+ dma->tx_err = 1;
+ p->ier |= UART_IER_THRI;
+ serial_port_out(&p->port, UART_IER, p->ier);
+ }
+ }
spin_unlock_irqrestore(&p->port.lock, flags);
}
@@ -69,6 +77,7 @@ int serial8250_tx_dma(struct uart_8250_port *p)
struct uart_8250_dma*dma = p->dma;
struct circ_buf *xmit = &p->port.state->xmit;
struct dma_async_tx_descriptor *desc;
+ int ret;
if (uart_tx_stopped(&p->port) || dma->tx_running ||
uart_circ_empty(xmit))
@@ -80,8 +89,10 @@ int serial8250_tx_dma(struct uart_8250_port *p)
dma->tx_addr + xmit->tail,
dma->tx_size, DMA_MEM_TO_DEV,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
- if (!desc)
- return -EBUSY;
+ if (!desc) {
+ ret = -EBUSY;
+ goto err;
+ }
dma->tx_running = 1;
@@ -94,8 +105,17 @@ int serial8250_tx_dma(struct uart_8250_port *p)
UART_XMIT_SIZE, DMA_TO_DEVICE);
dma_async_issue_pending(dma->txchan);
-
+ if (dma->tx_err) {
+ dma->tx_err = 0;
+ if (p->ier & UART_IER_THRI) {
+ p->ier &= ~UART_IER_THRI;
+ serial_out(p, UART_IER, p->ier);
+ }
+ }
return 0;
+err:
+ dma->tx_err = 1;
+ return ret;
}
EXPORT_SYMBOL_GPL(serial8250_tx_dma);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/13] tty: serial: 8250: Fix wording in runtime-PM comments
Frans reworded the two comments with better English for better
understanding. His review hit the mailing list after the patch got
applied so here is an incremental update.
Reported-by: Frans Klaver
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index 139f3d2b8aa9..a1904628a2a1 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -557,7 +557,7 @@ static void serial8250_rpm_put(struct uart_8250_port *p)
}
/*
- * This two wrapper ensure, that enable_runtime_pm_tx() can be called more than
+ * These two wrappers ensure that enable_runtime_pm_tx() can be called more
than
* once and disable_runtime_pm_tx() will still disable RPM because the fifo is
* empty and the HW can idle again.
*/
@@ -1532,7 +1532,7 @@ void serial8250_tx_chars(struct uart_8250_port *up)
DEBUG_INTR("THRE...");
/*
-* With RPM enabled, we have to wait once the FIFO is empty before the
+* With RPM enabled, we have to wait until the FIFO is empty before the
* HW can go idle. So we get here once again with empty FIFO and disable
* the interrupt and RPM in __stop_tx()
*/
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/13] tty: serial: 8250_omap: add custom DMA-TX callback
This patch provides mostly a copy of serial8250_tx_dma() +
__dma_tx_complete() with the following extensions:
- DMA bug
At least on AM335x the following problem exists: Even if the TX FIFO is
empty and a TX transfer is programmed (and started) the UART does not
trigger the DMA transfer.
After $TRESHOLD number of bytes have been written to the FIFO manually the
UART reevaluates the whole situation and decides that now there is enough
room in the FIFO and so the transfer begins.
This problem has not been seen on DRA7 or beagle board xm (OMAP3). I am not
sure if this is UART-IP core specific or DMA engine.
The workaround is to use a threshold of one byte, program the DMA
transfer minus one byte and then to put the first byte into the FIFO to
kick start the transfer.
- support for runtime PM
RPM is enabled on start_tx(). We can't disable RPM on DMA complete callback
because there is still data in the FIFO which is being sent. We have to wait
until the FIFO is empty before we disable it.
For this to happen we fake a TX sent error and enable THRI. Once the
FIFO is empty we receive an interrupt and since the TTY-buffer is still
empty we "put RPM" via __stop_tx(). Should it been filed then in the
start_tx() path we should program the DMA transfer and remove the error
flag and the THRI bit.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 144
include/uapi/linux/serial_reg.h | 1 +
2 files changed, 145 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 2f653c48639d..5f183d197dfa 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include "8250.h"
@@ -29,6 +30,7 @@
#define UART_ERRATA_i202_MDR1_ACCESS (1 << 0)
#define OMAP_UART_WER_HAS_TX_WAKEUP(1 << 1)
+#define OMAP_DMA_TX_KICK (1 << 2)
#define OMAP_UART_FCR_RX_TRIG 6
#define OMAP_UART_FCR_TX_TRIG 4
@@ -616,6 +618,148 @@ static void omap_8250_unthrottle(struct uart_port *port)
pm_runtime_put_autosuspend(port->dev);
}
+#ifdef CONFIG_SERIAL_8250_DMA
+static int omap_8250_tx_dma(struct uart_8250_port *p);
+
+static void omap_8250_dma_tx_complete(void *param)
+{
+ struct uart_8250_port *p = param;
+ struct uart_8250_dma*dma = p->dma;
+ struct circ_buf *xmit = &p->port.state->xmit;
+ unsigned long flags;
+ boolen_thri = false;
+
+ dma_sync_single_for_cpu(dma->txchan->device->dev, dma->tx_addr,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+
+ spin_lock_irqsave(&p->port.lock, flags);
+
+ dma->tx_running = 0;
+
+ xmit->tail += dma->tx_size;
+ xmit->tail &= UART_XMIT_SIZE - 1;
+ p->port.icount.tx += dma->tx_size;
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(&p->port);
+
+ if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port)) {
+ int ret;
+
+ ret = omap_8250_tx_dma(p);
+ if (ret)
+ en_thri = true;
+
+ } else if (p->capabilities & UART_CAP_RPM) {
+ en_thri = true;
+ }
+
+ if (en_thri) {
+ dma->tx_err = 1;
+ p->ier |= UART_IER_THRI;
+ serial_port_out(&p->port, UART_IER, p->ier);
+ }
+
+ spin_unlock_irqrestore(&p->port.lock, flags);
+}
+
+static int omap_8250_tx_dma(struct uart_8250_port *p)
+{
+ struct uart_8250_dma*dma = p->dma;
+ struct omap8250_priv*priv = p->port.private_data;
+ struct circ_buf *xmit = &p->port.state->xmit;
+ struct dma_async_tx_descriptor *desc;
+ unsigned intskip_byte = 0;
+ int ret;
+
+ if (dma->tx_running)
+ return 0;
+ if (uart_tx_stopped(&p->port) || uart_circ_empty(xmit)) {
+
+ /*
+* Even if no data, we need to return an error for the two cases
+* below so serial8250_tx_chars() is invoked and properly clears
+* THRI and/or runtime suspend.
+*/
+ if (dma->tx_err || p->capabilities & UART_CAP_RPM) {
+ ret = -EBUSY;
+ goto err;
+ }
+ if (p->ier & UART_IER_THRI) {
+ p->ier &= ~UART_IER_THRI;
+ serial_out(p, UART_IER, p->ier);
+ }
+ return 0;
+ }
+
+ dma->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE)
[PATCH 06/13] tty: serial: 8250: allow to use custom DMA implementation
The OMAP has a few corner cases where it needs a share of kindness of
affection to do the right thing. Heikki Krogerus suggested that instead
adding the quirks into the default DMA implementation, OMAP could get
its own copy of the function. And Alan suggested the same thing so here
we go.
This patch provides callbacks for custom TX/RX DMA implementation. If
there are not setup / used, then the default (current) implementation is
used.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 3 +++
drivers/tty/serial/8250/8250_core.c | 11 ---
drivers/tty/serial/8250/8250_dma.c | 2 --
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index ebab625179d4..4bb831ab5db0 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -16,6 +16,9 @@
#include
struct uart_8250_dma {
+ int (*tx_dma)(struct uart_8250_port *p);
+ int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
+
dma_filter_fn fn;
void*rx_param;
void*tx_param;
diff --git a/drivers/tty/serial/8250/8250_core.c
b/drivers/tty/serial/8250/8250_core.c
index ea57c87f8528..93b0799936fd 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1350,7 +1350,7 @@ static void serial8250_start_tx(struct uart_port *port)
struct uart_8250_port *up = up_to_u8250p(port);
serial8250_rpm_get_tx(up);
- if (up->dma && !serial8250_tx_dma(up)) {
+ if (up->dma && !up->dma->tx_dma(up)) {
return;
} else if (!(up->ier & UART_IER_THRI)) {
up->ier |= UART_IER_THRI;
@@ -1588,7 +1588,7 @@ int serial8250_handle_irq(struct uart_port *port,
unsigned int iir)
if (status & (UART_LSR_DR | UART_LSR_BI)) {
if (up->dma)
- dma_err = serial8250_rx_dma(up, iir);
+ dma_err = up->dma->rx_dma(up, iir);
if (!up->dma || dma_err)
status = serial8250_rx_chars(up, status);
@@ -3624,8 +3624,13 @@ int serial8250_register_8250_port(struct uart_8250_port
*up)
uart->dl_read = up->dl_read;
if (up->dl_write)
uart->dl_write = up->dl_write;
- if (up->dma)
+ if (up->dma) {
uart->dma = up->dma;
+ if (!uart->dma->tx_dma)
+ uart->dma->tx_dma = serial8250_tx_dma;
+ if (!uart->dma->rx_dma)
+ uart->dma->rx_dma = serial8250_rx_dma;
+ }
if (serial8250_isa_config != NULL)
serial8250_isa_config(0, &uart->port,
diff --git a/drivers/tty/serial/8250/8250_dma.c
b/drivers/tty/serial/8250/8250_dma.c
index db9eda3c12d6..258430b72039 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -118,7 +118,6 @@ int serial8250_tx_dma(struct uart_8250_port *p)
dma->tx_err = 1;
return ret;
}
-EXPORT_SYMBOL_GPL(serial8250_tx_dma);
int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
{
@@ -165,7 +164,6 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned
int iir)
return 0;
}
-EXPORT_SYMBOL_GPL(serial8250_rx_dma);
int serial8250_request_dma(struct uart_8250_port *p)
{
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/13] tty: serial: 8250_omap: add custom DMA-RX callback
The omap needs a DMA request pending right away. If it is
enqueued once the bytes are in the FIFO then nothing will happen
and the FIFO will be later purged via RX-timeout interrupt.
This patch enqueues RX-DMA request on completion but not if it
was aborted on error. The first enqueue will happen in the driver
in startup.
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250_omap.c | 96 +
1 file changed, 96 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 5f183d197dfa..1659858e595a 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -619,6 +619,102 @@ static void omap_8250_unthrottle(struct uart_port *port)
}
#ifdef CONFIG_SERIAL_8250_DMA
+static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir);
+
+static void __dma_rx_do_complete(struct uart_8250_port *p, bool error)
+{
+ struct uart_8250_dma*dma = p->dma;
+ struct tty_port *tty_port = &p->port.state->port;
+ struct dma_tx_state state;
+ int count;
+
+ dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
+ dma->rx_size, DMA_FROM_DEVICE);
+
+ dma->rx_running = 0;
+ dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
+ dmaengine_terminate_all(dma->rxchan);
+
+ count = dma->rx_size - state.residue;
+
+ tty_insert_flip_string(tty_port, dma->rx_buf, count);
+ p->port.icount.rx += count;
+ if (!error)
+ omap_8250_rx_dma(p, 0);
+
+ tty_flip_buffer_push(tty_port);
+}
+
+static void __dma_rx_complete(void *param)
+{
+ __dma_rx_do_complete(param, false);
+}
+
+static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
+{
+ struct uart_8250_dma*dma = p->dma;
+ struct dma_async_tx_descriptor *desc;
+
+ switch (iir & 0x3f) {
+ case UART_IIR_RLSI:
+ /* 8250_core handles errors and break interrupts */
+ if (dma->rx_running) {
+ dmaengine_pause(dma->rxchan);
+ __dma_rx_do_complete(p, true);
+ }
+ return -EIO;
+ case UART_IIR_RX_TIMEOUT:
+ /*
+* If RCVR FIFO trigger level was not reached, complete the
+* transfer and let 8250_core copy the remaining data.
+*/
+ if (dma->rx_running) {
+ dmaengine_pause(dma->rxchan);
+ __dma_rx_do_complete(p, true);
+ }
+ return -ETIMEDOUT;
+ case UART_IIR_RDI:
+ /*
+* The OMAP UART is a special BEAST. If we receive RDI we _have_
+* a DMA transfer programmed but it didn't work. One reason is
+* that we were too slow and there were too many bytes in the
+* FIFO, the UART counted wrong and never kicked the DMA engine
+* to do anything. That means once we receive RDI on OMAP then
+* the DMA won't do anything soon so we have to cancel the DMA
+* transfer and purge the FIFO manually.
+*/
+ if (dma->rx_running) {
+ dmaengine_pause(dma->rxchan);
+ __dma_rx_do_complete(p, true);
+ }
+ return -ETIMEDOUT;
+
+ default:
+ break;
+ }
+
+ if (dma->rx_running)
+ return 0;
+
+ desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
+ dma->rx_size, DMA_DEV_TO_MEM,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!desc)
+ return -EBUSY;
+
+ dma->rx_running = 1;
+ desc->callback = __dma_rx_complete;
+ desc->callback_param = p;
+
+ dma->rx_cookie = dmaengine_submit(desc);
+
+ dma_sync_single_for_device(dma->rxchan->device->dev, dma->rx_addr,
+ dma->rx_size, DMA_FROM_DEVICE);
+
+ dma_async_issue_pending(dma->rxchan);
+ return 0;
+}
+
static int omap_8250_tx_dma(struct uart_8250_port *p);
static void omap_8250_dma_tx_complete(void *param)
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/13] tty: serial: 8250_dma: keep own book keeping about RX transfers
After dmaengine_terminate_all() has been invoked then both DMA drivers
(edma and omap-dma) do not invoke dma_cookie_complete() to mark the
transfer as complete. This dma_cookie_complete() is performed by the
Synopsys DesignWare driver which is probably the only one that is used
by omap8250-dma and hence don't see following problem…
…which is that once a RX transfer has been terminated then following
query of channel status reports DMA_IN_PROGRESS (again: the actual
transfer has been canceled, there is nothing going on anymore).
This means that serial8250_rx_dma() never enqueues another DMA transfer
because it (wrongly) assumes that there is a transer already pending.
Vinod Koul refuses to accept a patch which adds this
dma_cookie_complete() to both drivers and so dmaengine_tx_status() would
report DMA_COMPLETE instead (and behave like the Synopsys DesignWare
driver already does). He argues that I am not allowed to use the cookie
to query the status and that the driver already cleaned everything up after
the invokation of dmaengine_terminate_all().
To end this I add a bookkeeping whether or not a RX-transfer has been
started to the 8250-dma code. It has already been done for the TX side.
*Now* we learn about the RX status based on our bookkeeping and don't
need dmaengine_tx_status() for this anymore.
Cc: vinod.k...@intel.com
Reviewed-by: Tony Lindgren
Tested-by: Tony Lindgren
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/tty/serial/8250/8250.h | 1 +
drivers/tty/serial/8250/8250_dma.c | 10 --
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index a63d198f8d03..ebab625179d4 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -42,6 +42,7 @@ struct uart_8250_dma {
unsigned char tx_running:1;
unsigned char tx_err: 1;
+ unsigned char rx_running:1;
};
struct old_serial_port {
diff --git a/drivers/tty/serial/8250/8250_dma.c
b/drivers/tty/serial/8250/8250_dma.c
index 69e54abb6e71..db9eda3c12d6 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -61,6 +61,7 @@ static void __dma_rx_complete(void *param)
dma_sync_single_for_cpu(dma->rxchan->device->dev, dma->rx_addr,
dma->rx_size, DMA_FROM_DEVICE);
+ dma->rx_running = 0;
dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
dmaengine_terminate_all(dma->rxchan);
@@ -123,10 +124,6 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned
int iir)
{
struct uart_8250_dma*dma = p->dma;
struct dma_async_tx_descriptor *desc;
- struct dma_tx_state state;
- int dma_status;
-
- dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
switch (iir & 0x3f) {
case UART_IIR_RLSI:
@@ -137,7 +134,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned
int iir)
* If RCVR FIFO trigger level was not reached, complete the
* transfer and let 8250_core copy the remaining data.
*/
- if (dma_status == DMA_IN_PROGRESS) {
+ if (dma->rx_running) {
dmaengine_pause(dma->rxchan);
__dma_rx_complete(p);
}
@@ -146,7 +143,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned
int iir)
break;
}
- if (dma_status)
+ if (dma->rx_running)
return 0;
desc = dmaengine_prep_slave_single(dma->rxchan, dma->rx_addr,
@@ -155,6 +152,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned
int iir)
if (!desc)
return -EBUSY;
+ dma->rx_running = 1;
desc->callback = __dma_rx_complete;
desc->callback_param = p;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/13] dmaengine: edma: check for echan->edesc => NULL in edma_dma_pause()
I added book keeping of whether or not the 8250-dma driver has an RX
transfer pending or not so we don't BUG here if it calls
dmaengine_pause() on a channel which has not a pending transfer. Guess
what, this is not enough.
The following can be triggered with a busy RX channel and hackbench in
background:
- DMA transfer completes. The callback is delayed via
vchan_cookie_complete() into a tasklet so it das not happen asap.
- hackbench keeps the system busy so the tasklet does not run "soon".
- the UART collected enough data and generates an "timeout"-interrupt.
Since 8250-dma *thinks* the DMA-transfer is still pending it tries to
cancel it via invoking dmaengine_pause() first. This causes the segfault
because echan->edesc is NULL now that the transfer completed (however
the callback did not run yet).
With this patch we don't BUG in the scenario described.
Cc: vinod.k...@intel.com
Signed-off-by: Sebastian Andrzej Siewior
---
drivers/dma/edma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7b65633f495e..123f578d6dd3 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -288,7 +288,7 @@ static int edma_slave_config(struct edma_chan *echan,
static int edma_dma_pause(struct edma_chan *echan)
{
/* Pause/Resume only allowed with cyclic mode */
- if (!echan->edesc->cyclic)
+ if (!echan->edesc || !echan->edesc->cyclic)
return -EINVAL;
edma_pause(echan->ch_num);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/13 v10] omap 8250 based UART + DMA
The queue is getting smaller. The highlights of v9…v10 - the DMA stall Frans Klaver reported which popped up in yocto is gone. It also seems that the "ack the err-irq even if nothing happened" in EDMA can be dropped. - the RX- and TX-DMA callbacks are now OMAP-only and no "bugs" flags are introduced into the generic DMA code. This also means that there is custom IRQ routine in case of DMA. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Peter Hurley | 2014-09-17 08:20:41 [-0400]: >> So I connected my am335x-evm >> with beagle board xm because both of them have an old fashion UART >> connector (instead those uart-to-usb). Both configured with HW-Flow and >> I haven't seen the function invoked but I saw "port->icount.overrun" >> being incremented. This shouldn't happen. So I am a little puzzled here… > >Yeah, that's weird. Do you have a break-out box to confirm that RTS/CTS are >being driven? => - beagle board According to schematics the board has only RX and TX connected. No RTS/CTS - am335x-evm The schematics say "DNI" next to a resistor on RTS/CTS. DNI stands most likely for "Do Not Install". With a scope it looks like the the wire behind the MAX is open. - beagle bone black Only RX and TX are wired towards the USB2serial device. In short: each device I have has RTS/CTS not working/connected and I can't test HW handshake with HW available. >Regards, >Peter Hurley Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/16] tty: serial: 8250_core: add run time pm
* Frans Klaver | 2014-09-29 11:46:20 [+0200]:
>On Wed, Sep 10, 2014 at 09:29:57PM +0200, Sebastian Andrzej Siewior wrote:
>> +/*
>> + * This two wrapper ensure, that enable_runtime_pm_tx() can be called more
>> than
>
>These two wrappers ensure that enable_runtime_pm_tx() ...
fixed
>> @@ -1469,7 +1531,12 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>>
>> DEBUG_INTR("THRE...");
>>
>> -if (uart_circ_empty(xmit))
>> +/*
>> + * With RPM enabled, we have to wait once the FIFO is empty before the
>
>s,once,until,? Or do I not understand the sentence correctly?
No, you understand it correct. "until" is the better word here.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/16] tty: serial: Add 8250-core based omap driver
* Frans Klaver | 2014-09-29 11:38:23 [+0200]: >threshold fixed >> +/* >> + * It claims to be 16C750 compatible however it is a little different. >> + * It has EFR and has no FCR7_64byte bit. The AFE (which it claims to >> + * have) is enabled via EFR instead of MCR. The type is set here 8250 >> + * just to get things going. UNKNOWN does not work for a few reasons and >> + * we don't need our own type since we don't use 8250's set_termios() >> + * and our "bugs" are handeld via the bug member. > >handled replaced that last line with or pm callback. since there no bugs member anymore. > >> + */ >> +up.port.type = PORT_8250; >> +up.port.iotype = UPIO_MEM; >> +up.port.flags = UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_SOFT_FLOW | >> +UPF_HARD_FLOW; >> +up.port.private_data = priv; >> + >> +up.port.regshift = 2; >> +up.port.fifosize = 64; >> +up.tx_loadsz = 64; >> +up.capabilities = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP; >> +#ifdef CONFIG_PM_RUNTIME >> +/* >> + * PM_RUNTIME is mostly transparent. However to do it right we need to a > >need to _do_ a ...? I think dropping that 'to' should fix it. >> + * TX empty interrupt before we can put the device to auto idle. So if >> + * PM_RUNTIME is not enabled we don't add that flag and can spare that >> + * one extra interrupt in the TX path. >> + */ > > > >> +config SERIAL_8250_OMAP >> +tristate "Support for OMAP internal UART (8250 based driver)" >> +depends on SERIAL_8250 && ARCH_OMAP2PLUS >> +help >> + If you have a machine based on an Texas Instruments OMAP CPU you >> + can enable its onboard serial ports by enabling this option. >> + >> + This driver is in early stage and uses ttyS instead of ttyO. >> + > >I just wondered if this driver should be marked experimental? What did you have in mind? CONFIG_EXPERIMENTAL is gone. After all that debuging that I had in the meantime I was thinking about dropping that "early stage". >Thanks, >Frans Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/16] tty: serial: 8250_dma: add pm runtime
* Frans Klaver | 2014-09-29 11:26:06 [+0200]: >On Wed, Sep 10, 2014 at 09:30:08PM +0200, Sebastian Andrzej Siewior wrote: >> There is nothing to do for RPM in the RX path. If the HW goes off then it >> won't assert the DMA line and the transfer won't happen. So we hope that >> the HW does not go off for RX to work (DMA or PIO makes no difference >> here). >> >> For TX the situation is slightly different. RPM is enabled on >> start_tx(). We can't disable RPM on DMA complete callback because there >> is still data in the FIFO which is being sent. We have to wait until >> the FIFO is empty before we disable it. >> For this to happen we fake a TX sent error and enable THRI. Once the >> FIFO is empty we receive an interrupt and since the TTY-buffer is still >> empty we "put RPM" via __stop_tx(). Should it been filed then in the >> start_tx() path we should program the DMA transfer and remove the error >> flag and the THRI bit. > >That last sentence starts out a bit messy. This got mered so there is nothing I can do about it anymore. But I will try to fix comments in code where and in patches that are not yet merged (what you report :)) Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Frans Klaver | 2014-09-29 10:50:42 [+0200]: >This version fixes the console things for us. It also increases the >amount of data we can push over the serial port. If I push the data >towards our requirements, we're not there yet. I get the "too much work >for irq" notice still. However, I don't think you'd need to be fixing >that in this series (or at all). We had similar issues there with >omap-serial as well. > >As far as I'm concerned, this is > >Tested-by: Frans Klaver Thanks a lot. There is a patch named "ARM: edma: unconditionally ack the error interrupt". I have the feeling that this is not really required once we delay set_termios. I couldn't reproduce the bug with beagleblack with my usual test case. For your "too much work for irq" problem: Could you add trace_printk() in tx/rx dma start/complete, and irq routine? The interresting part is what is the irq routine doing once entered. It might be a condition that is ignored at first and "acked" later while serving another event. Or it is really doing something and this is more or less "legal". >Thanks, >Frans Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] ARM: edma: unconditionally ack the error interrupt
* Peter Ujfalusi | 2014-09-20 00:29:01 [+0300]: >mask 800 means URXEVT0 (UART0 rx), 400 is UTXEVT0 (UART0 tx) events. >But it is clear that my theory was not even close to what's going on. >Do you have some tool which can be used to reproduce this issue? The latest uart patch set is at git://git.breakpoint.cc/bigeasy/linux.git uart_v10_pre3 and the latest one I tested and saw the issue was uart_v9. The test case is, boot, login via serial, do "less file" and press space for a while to get less to scroll the file. The error counter increment after a while. >> Fun fact: If I remove the write access to EDMA_EMCR register (the write >> access after the read out) then I haven't seen [1] a single error interrupt >> beeing "unhandled" out of 9. The former has three out of eight. > >Hrm, this is interesting. Am I interpret it right: >if you clear the event missed register's bit for the event, you will receive >the interrupt, but there if you read the EMR bits, they are all 0. usually yes. Sometimes the EMR bits are set. >if you do not clear the bit in the event missed register (even if it is not >set) you will not receive the error interrupt. Right? no, the error interrupt comes but the EMR is always non-zero. >I think this can be due to the fact how edma (if I read the TRM right) works: >the error irq will be asserted if there is a transition from 0 to 1 in one of >the error registers. If you had error and you did not cleared the error bit, >the next error will not going to cause another transition so no interrupt will >be triggered. Having said that, there should be at least one error interrupt >coming since at some point we should have had 0 -> 1 transition... > >Can you print out also the EDMA_EMR at places you were printing EDMA_EMCR? So I had a "bug" where I changed the baud + DMA bits in the UART while there was a DMA-TX in progress. Now I dropped this in v10_pre3 and it seems not to trigger anymore. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Frans Klaver | 2014-09-22 11:28:54 [+0200]: >I guess then we'd still have to answer the question why the yocto build >calls set_termios() so often, but that's not on you then. Did you notice >it even changing settings? We might want to fix this even if the kernel >should be able to cope. could you please test uart_v10_pre3? I dropped a few register sets and delayed the register update until TX-DMA is complete. I am traveling currently and have only my boneblack and it passes my limited testing. If it solves your problems, too then I would address Heikki's latest comments and prepare the final v10 (and I hope I didn't break beagle board in between). >Thanks again, >Frans Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Peter Hurley | 2014-09-25 07:31:32 [-0400]:
>I just verified that GNU readline6 uses ioctl(TCSETSW, ...) to do the
>set_termios
>(which is the ioctl that libc should use for tcsetattr(TCSADRAIN)).
>
>Maybe this userspace is using a readline()-alike that has a bug by not using
>the
>correct tcsetattr() action?
set_termios() has an opt argument. While doing ":n" in vi I see two invocations
with "opt == 8" which stands for TCSETS.
browsing through vi's code I stumbled upon
|static void rawmode(void)
|{
…
|tcsetattr_stdin_TCSANOW(&term_vi);
|}
|int FAST_FUNC tcsetattr_stdin_TCSANOW(const struct termios *tp)
|{
|return tcsetattr(STDIN_FILENO, TCSANOW, tp);
|}
and this is probably what you meant. There is also
| static void cookmode(void)
| {
| fflush_all();
| tcsetattr_stdin_TCSANOW(&term_orig);
| }
However I don't see __tty_perform_flush() in kernel invoked.
>Or maybe the glibc-equivalent has the bug, and tcsetattr(TCSADRAIN) is not
>using
>ioctl(TCSETSW)?
libc is "GNU C Library 2.20".
>Regards,
>Peter Hurley
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Sebastian Andrzej Siewior | 2014-09-24 09:53:46 [+0200]:
>* Peter Hurley | 2014-09-23 13:03:51 [-0400]:
>
>>But DMA is cheating if the UART driver's tx_empty() method is saying the
>>transmitter is empty while TX DMA is still running.
>This shouldn't be the case. But I will check this once I able to.
I added
|#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
|trace_printk("delay <%d>\n", (lsr & BOTH_EMPTY) == BOTH_EMPTY ? 1 : 0);
in my set_termios() and the trace shows
| vi-949 [000] d...70.477002: omap8250_restore_regs: delay
<0>
so no, it does not wait until TX FIFO is empty. It looks like it uses
TCSANOW instead of TCSADRAIN. And since this looks "legal" I will delay
it until TX-DMA is complete because it is known to stall the DMA operation.
>>Regards,
>>Peter Hurley
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Heikki Krogerus | 2014-09-22 10:46:05 [+0300]: >Well, there are no other SoCs at the moment that would need it, and >it's actually possible that there never will be. So yes, just handle >that also in the omap specific code. Okay. So let me move the irq routine and the workarounds into omap then. >Thanks, > Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Frans Klaver | 2014-09-22 11:28:54 [+0200]: > >Wow, thanks for your work here. This does indeed sound hard to trap. > >I guess then we'd still have to answer the question why the yocto build >calls set_termios() so often, but that's not on you then. Did you notice >it even changing settings? We might want to fix this even if the kernel >should be able to cope. Yeah. I don't care if this is yocto's fault or not. If it is possible to stop the DMA transfer from userland with whatever method then it needs to be fixed in kernel so that this does happen. >Thanks again, >Frans Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Peter Hurley | 2014-09-23 13:03:51 [-0400]: >readline() does this; it 'saves' the caller's termios, sets termios >for non-canonical reads, reads one char, and 'restores' the caller's >termios. interresting, thanks. I guess I would need to opimize this a little so the baudrate isn't going to 0 and back to the requested baudrate. >The tty core calls the driver's wait_until_sent() method before changing >the termios (if TCSADRAIN is used for tcsetattr(), which I think for readline() >it does). The interresting difference is that when I take the yocto RFS and chroot into from Debian then I don't this problem. Not sure if this is really readline or something else… >But DMA is cheating if the UART driver's tx_empty() method is saying the >transmitter is empty while TX DMA is still running. This shouldn't be the case. But I will check this once I able to. After TX-DMA is done, "xmit->tail" is updated and port.icount.tx is incremented. At this time the TX FIFO is still full (up to 64 bytes) and I set UART_IER_THRI to wait until TX FIFO is empty so I can disable runtime-pm. Therefore I would assume LSR does not say BOTH_EMPTY until the FIFO is empty. >Regards, >Peter Hurley Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/16] tty: serial: 8250_dma: Add a TX trigger workaround for AM33xx
* Frans Klaver | 2014-09-17 12:28:12 [+0200]: >- Bone Black: Yocto poky, core-image-minimal > Login, "less file" locks up, doesn't show anything. I can exit using > Ctrl-C. So I have the same with my and the serial-omap driver. No difference here. The trace looks like this: | -0 [000] d.h. 444.393585: serial8250_handle_irq: iir cc lsr 61 | -0 [000] d.h. 444.393605: serial8250_rx_chars: get 0d received the enter key | -0 [000] d.h. 444.393609: serial8250_rx_chars: insert d lsr 61 | -0 [000] d.h. 444.393614: uart_insert_char: 1 | -0 [000] d.h. 444.393617: uart_insert_char: 2 | -0 [000] dnh. 444.393636: serial8250_tx_chars: empty | kworker/0:2-753 [000] d... 444.393686: serial8250_start_tx: empty?1 | kworker/0:2-753 [000] d.h. 444.393699: serial8250_handle_irq: iir c2 lsr 60 | kworker/0:2-753 [000] d.h. 444.393705: serial8250_tx_chars: empty | sh-1042 [000] d... 444.393822: serial8250_start_tx: empty?1 | sh-1042 [000] d.h. 444.393836: serial8250_handle_irq: iir c2 lsr 60 | sh-1042 [000] d.h. 444.393842: serial8250_tx_chars: empty | sh-1042 [000] d... 444.393855: serial8250_start_tx: empty?0 | sh-1042 [000] d.h. 444.393863: serial8250_handle_irq: iir c2 lsr 60 | sh-1042 [000] d.h. 444.393867: serial8250_tx_chars: put 0d | sh-1042 [000] d.h. 444.393871: serial8250_tx_chars: put 0a shell responded with "\r\n" which I see and then | sh-1042 [000] d.h. 444.394057: serial8250_handle_irq: iir c2 lsr 60 | sh-1042 [000] d.h. 444.394065: serial8250_tx_chars: empty nothing more. less isn't sending data for some reason. Exactly the same thing happens in a Debian environment except that it continues: … | bash-2468 [000] d.h.99.657899: serial8250_tx_chars: put 0a | bash-2468 [000] d.h.99.658089: serial8250_handle_irq: iir c2 lsr 60 | bash-2468 [000] d.h.99.658095: serial8250_tx_chars: empty => | less-2474 [000] d...99.696038: serial8250_start_tx: empty?0 | less-2474 [000] d.h.99.696069: serial8250_handle_irq: iir c2 lsr 60 | less-2474 [000] d.h.99.696078: serial8250_tx_chars: put 1b | less-2474 [000] d.h.99.696082: serial8250_tx_chars: put 5b | less-2474 [000] d.h.99.696085: serial8250_tx_chars: put 3f | less-2474 [000] d.h.99.696087: serial8250_tx_chars: put 31 It has to be something about the environment. Booting Debian and chroot into this RFS and less works perfectly. But since it behaves like that with both drivers, I guess the problem is somewhere else… > vi runs normally, only occupies part of the total screen estate in > minicom. After quitting, a weird character shows up (typically I see > ÿ there), but minicom can use the rest of the screen estate again. > If we disregard the odd character, this is much like the behavior we > have on the omap-serial driver. >- Custom board: Yocto poky, custom image > Login, "less file" locks up, showing only "ÿ" in the top left corner > of the screen. Can get out of there by having something dumped through > /dev/kmsg. I managed to run into something like that with vi on dra7 and with little more patience on am335x as well by "vi *" and then ":n". This gets fixed indeed by writing. Hours of debugging and a lot of hair less later: the yocto RFS calls set_termios quite a lot. This includes changing the baudrate (not by yocto but the driver sets it to 0 and then to the requested one) and this seems to be responsible for the "bad bytes". I haven't figured out yet I don't see this with omap-serial. Even worse: If this (set_termios()) happens while the DMA is still active then it might stall it. A write into the FIFO seems to fix it and this is where your "echo >/dev/kmsg" fixes things. If I delay the restore_registers part of set_termios() until TX-DMA is complete then it seems that the TX-DMA stall does not tall anymore. >Having it summed up like this, I think we're back at ncurses and its >interaction with the serial driver. > >Hope this helps. Thanks for your effort so far, >Frans Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
