| Message ID | 1460061433-63750-13-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote:
Preface. I tried this on Galileo and it appears to work. I'll do some
throughput testing to verify but, initially the results are positive :)
> + lpss->dma_maxburst = 8;
Are these dwords ? If those are bytes then the maxburst value looks
small. In the BSP the max burst is 32 bytes.
---
bod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: > On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote: > > Preface. I tried this on Galileo and it appears to work. I'll do some > throughput testing to verify but, initially the results are positive :) I submitted (and pushed into my branch) a bit changed version (see my v2). >> + lpss->dma_maxburst = 8; > > Are these dwords ? If those are bytes then the maxburst value looks > small. In the BSP the max burst is 32 bytes. max_burst is in items of given size (here is 32 bytes for memory and 8 bytes for UART). I took this value from Quark BSP, but I'll be happy to adjust to more optimal values.
On Mon, Apr 11, 2016 at 6:51 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue > <pure.logic@nexus-software.ie> wrote: >> On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote: >> >> Preface. I tried this on Galileo and it appears to work. I'll do some >> throughput testing to verify but, initially the results are positive :) > > I submitted (and pushed into my branch) a bit changed version (see my v2). > >>> + lpss->dma_maxburst = 8; >> >> Are these dwords ? If those are bytes then the maxburst value looks >> small. In the BSP the max burst is 32 bytes. > > max_burst is in items of given size (here is 32 bytes for memory and 8 > bytes for UART). I took this value from Quark BSP, but I'll be happy > to adjust to more optimal values. +Jarkko. Oy vei, seems we don't set memory side of transfer neither in 8250_dma, nor in spi-pxa2xx-dma.
On Mon, 2016-04-11 at 18:51 +0300, Andy Shevchenko wrote: > On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue > <pure.logic@nexus-software.ie> wrote: > > On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote: > > > > Preface. I tried this on Galileo and it appears to work. I'll do > > some > > throughput testing to verify but, initially the results are > > positive :) > > I submitted (and pushed into my branch) a bit changed version (see my > v2). > > > > + lpss->dma_maxburst = 8; > > > > Are these dwords ? If those are bytes then the maxburst value looks > > small. In the BSP the max burst is 32 bytes. > > max_burst is in items of given size (here is 32 bytes for memory and > 8 I haven't read your V2 yet but on this, I'd suggest raising the burst size to 32 bytes for UART (no higher) we found during bringup that larger sizes "fall-over and die" but, anything up to 32 bytes is OK - and therefore you should be able to reduce the number of bursts/interrupts etc. --- bod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-04-12 at 17:25 +0100, Bryan O'Donoghue wrote: > On Mon, 2016-04-11 at 18:51 +0300, Andy Shevchenko wrote: > > > > On Mon, Apr 11, 2016 at 6:33 PM, Bryan O'Donoghue > > <pure.logic@nexus-software.ie> wrote: > > > > > > On Thu, 2016-04-07 at 23:37 +0300, Andy Shevchenko wrote: > > > > > > Preface. I tried this on Galileo and it appears to work. I'll do > > > some > > > throughput testing to verify but, initially the results are > > > positive :) > > I submitted (and pushed into my branch) a bit changed version (see > > my > > v2). > > > > > > > > > > > > > + lpss->dma_maxburst = 8; > > > Are these dwords ? If those are bytes then the maxburst value > > > looks > > > small. In the BSP the max burst is 32 bytes. > > max_burst is in items of given size (here is 32 bytes for memory > > and > > 8 > I haven't read your V2 yet but on this, I'd suggest raising the burst > size to 32 bytes for UART (no higher) we found during bringup that > larger sizes "fall-over and die" but, anything up to 32 bytes is OK - > and therefore you should be able to reduce the number of > bursts/interrupts etc. It can't be more that FIFO size and recommendation as far as I know is FIFO/2, which is exactly 8 bytes.
On Tue, 2016-04-12 at 19:50 +0300, Andy Shevchenko wrote: > > I haven't read your V2 yet but on this, I'd suggest raising the > burst > > size to 32 bytes for UART (no higher) we found during bringup that > > larger sizes "fall-over and die" but, anything up to 32 bytes is OK > - > > and therefore you should be able to reduce the number of > > bursts/interrupts etc. > > It can't be more that FIFO size and recommendation as far as I know > is > FIFO/2, which is exactly 8 bytes. Why not ? We went as high as 32 bytes previously in the BSP with no obvious errors. At 8 bytes or 1/2 of the FIFO size I'd ask the question is DMA even worth it i.e. does it take more time to setup and execute a DMA transaction @ 1/2 FIFO size than just writing straight into the FIFO ? --- bod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-04-13 at 12:22 +0100, Bryan O'Donoghue wrote: > On Tue, 2016-04-12 at 19:50 +0300, Andy Shevchenko wrote: > > > > > > > > I haven't read your V2 yet but on this, I'd suggest raising the > > burst > > > > > > size to 32 bytes for UART (no higher) we found during bringup that > > > larger sizes "fall-over and die" but, anything up to 32 bytes is > > > OK > > - > > > > > > and therefore you should be able to reduce the number of > > > bursts/interrupts etc. > > It can't be more that FIFO size and recommendation as far as I know > > is > > FIFO/2, which is exactly 8 bytes. > Why not ? Because a probability of FIFO overrun. There is a big chapter ("Peripheral Burst Transaction Requests") in dw_apb_dmac_db.pdf covering this. > > We went as high as 32 bytes previously in the BSP with no obvious > errors. > > At 8 bytes or 1/2 of the FIFO size I'd ask the question is DMA even > worth it i.e. does it take more time to setup and execute a DMA > transaction @ 1/2 FIFO size than just writing straight into the FIFO ?
On Wed, 2016-04-13 at 15:03 +0300, Andy Shevchenko wrote: > Because a probability of FIFO overrun. > > There is a big chapter ("Peripheral Burst Transaction Requests") in > dw_apb_dmac_db.pdf covering this. I thought there was flow control between the controller and the FIFO here ? I don't have the spec SoC spec for the UART to hand but, if memory serves... -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-04-13 at 15:34 +0100, Bryan O'Donoghue wrote: > On Wed, 2016-04-13 at 15:03 +0300, Andy Shevchenko wrote: > > > > Because a probability of FIFO overrun. > > > > There is a big chapter ("Peripheral Burst Transaction Requests") in > > dw_apb_dmac_db.pdf covering this. > I thought there was flow control between the controller and the FIFO > here ? I don't have the spec SoC spec for the UART to hand but, if > memory serves... Wait, you mean flow control between DMA controller and UART FIFO, or I misread you?
On Wed, 2016-04-13 at 17:48 +0300, Andy Shevchenko wrote: > Wait, you mean flow control between DMA controller and UART FIFO, or > I > misread you? Yup. It's a while since I read the spec and talked to the relevant people but... I have this memory that the FIFO fill signal and DMA block were 'wired up' @ the AHB level. That would be how the UART and DMA block would flow-control each other for descriptor chaining at any rate and so one assumes that its active at the block-to-fifo layer. Meh I don't have the UART EAS anymore to comment in detail.. I think the right thing to do is to be safe (so I'll ACK your series) and then run an experiment to push the burst size upwards. If you have the EAS handy though it might be worthwhile working out when the DMA block will flow-control w/r to the FIFO fill level - I *think* (but can't prove since I don't have the EAS anymore) that it's safe to push that value higher. --- bod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c index 1b36d32..bc55738 100644 --- a/drivers/tty/serial/8250/8250_lpss.c +++ b/drivers/tty/serial/8250/8250_lpss.c @@ -15,7 +15,7 @@ #include <linux/rational.h> #include <linux/dmaengine.h> -#include <linux/platform_data/dma-dw.h> +#include <linux/dma/dw.h> #include "8250.h" @@ -47,6 +47,7 @@ struct lpss8250_board { unsigned long freq; unsigned int base_baud; int (*setup)(struct lpss8250 *, struct uart_port *p); + void (*exit)(struct lpss8250 *); }; struct lpss8250 { @@ -55,6 +56,7 @@ struct lpss8250 { /* DMA parameters */ struct uart_8250_dma dma; + struct dw_dma_chip dma_chip; struct dw_dma_slave dma_param; u8 dma_maxburst; }; @@ -141,17 +143,60 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port) return 0; } +static const struct dw_dma_platform_data qrk_serial_dma_pdata = { + .nr_channels = 2, + .is_private = true, + .is_nollp = true, + .chan_allocation_order = CHAN_ALLOCATION_ASCENDING, + .chan_priority = CHAN_PRIORITY_ASCENDING, + .block_size = 4095, + .nr_masters = 1, + .data_width = 4, +}; + static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port) { + struct uart_8250_dma *dma = &lpss->dma; + struct dw_dma_chip *chip = &lpss->dma_chip; + struct dw_dma_slave *param = &lpss->dma_param; struct pci_dev *pdev = to_pci_dev(port->dev); + int ret; pci_enable_msi(pdev); port->irq = pdev->irq; + chip->dev = &pdev->dev; + chip->irq = pdev->irq; + chip->regs = pci_ioremap_bar(pdev, 1); + chip->pdata = &qrk_serial_dma_pdata; + + /* Falling back to PIO mode if DMA probing fails */ + ret = dw_dma_probe(chip); + if (ret) + return 0; + + /* Special DMA address for UART */ + dma->dma_addr = 0xfffff000; + + param->dma_dev = &pdev->dev; + param->src_id = 0; + param->dst_id = 1; + param->polarity = true; + + lpss->dma_maxburst = 8; return 0; } +static void qrk_serial_exit(struct lpss8250 *lpss) +{ + struct dw_dma_slave *param = &lpss->dma_param; + + if (!param->dma_dev) + return; + dw_dma_remove(&lpss->dma_chip); +} + /*****************************************************************************/ static bool lpss8250_dma_filter(struct dma_chan *chan, void *param) @@ -243,22 +288,30 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id) ret = lpss8250_dma_setup(lpss, &uart); if (ret) - return ret; + goto err_exit; ret = serial8250_register_8250_port(&uart); if (ret < 0) - return ret; + goto err_exit; lpss->line = ret; pci_set_drvdata(pdev, lpss); return 0; + +err_exit: + if (lpss->board->exit) + lpss->board->exit(lpss); + return ret; } static void lpss8250_remove(struct pci_dev *pdev) { struct lpss8250 *lpss = pci_get_drvdata(pdev); + if (lpss->board->exit) + lpss->board->exit(lpss); + serial8250_unregister_port(lpss->line); } @@ -272,6 +325,7 @@ static const struct lpss8250_board qrk_board = { .freq = 44236800, .base_baud = 2764800, .setup = qrk_serial_setup, + .exit = qrk_serial_exit, }; #define LPSS_DEVICE(id, board) { PCI_VDEVICE(INTEL, id), (kernel_ulong_t)&board }
DMA on Intel Quark SoC is a part of UART IP block. Enable it. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/tty/serial/8250/8250_lpss.c | 60 +++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 3 deletions(-)