* [00/12] DMAENGINE: PL08X driver updates @ 2010-08-31 12:12 Linus Walleij 2010-08-31 12:12 ` [PATCH 01/12] DMAENGINE: remove clock code from PL08X driver Linus Walleij 2010-09-08 14:26 ` [00/12] DMAENGINE: PL08X driver updates Linus Walleij 0 siblings, 2 replies; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel Hi Dan, This updates the PL08X driver in the async_tx next branch with a number of fixes and features, each one pretty self-describing. This has been tested with the PrimeCell DMA series on the RealView PB11MPCore and works flawlessly, multiplexing several virtual clients, both devices (slaves) and memcpy channels, across only two (!) available physical channels. If you prefer you can squash these into the initial driver submission, I don't mind. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 01/12] DMAENGINE: remove clock code from PL08X driver 2010-08-31 12:12 [00/12] DMAENGINE: PL08X driver updates Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 02/12] DMAENGINE: safeguard PL08X from missing platform data Linus Walleij 2010-09-08 14:26 ` [00/12] DMAENGINE: PL08X driver updates Linus Walleij 1 sibling, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij With the introduction of the pclk infrastructure the block clock is handled by the AMBA PrimeCell bus core, and we can remove the local references to any clocks. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 18 ------------------ 1 files changed, 0 insertions(+), 18 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index fc5aaeb..2afb4f6 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -68,7 +68,6 @@ #include <linux/amba/bus.h> #include <linux/dmaengine.h> #include <linux/amba/pl08x.h> -#include <linux/clk.h> #include <asm/hardware/pl080.h> #include <asm/dma.h> @@ -113,7 +112,6 @@ struct lli { * @adev: the corresponding AMBA (PrimeCell) bus entry * @vd: vendor data for this PL08x variant * @pd: platform data passed in from the platform/machine - * @clk: a clock to be enabled for this device to run * @phy_chans: array of data for the physical channels * @pool: a pool for the LLI descriptors * @pool_ctr: counter of LLIs in the pool @@ -124,7 +122,6 @@ struct pl08x_driver_data { struct amba_device *adev; struct vendor_data *vd; struct pl08x_platform_data *pd; - struct clk *clk; struct pl08x_phy_chan *phy_chans; struct dma_pool *pool; int pool_ctr; @@ -1826,19 +1823,6 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) pl08x->adev = adev; pl08x->vd = vd; - pl08x->clk = clk_get(&adev->dev, NULL); - if (IS_ERR(pl08x->clk)) { - ret = PTR_ERR(pl08x->clk); - if (ret == -ENOENT) { - dev_info(&adev->dev, "no clock supplied, assume always on\n"); - pl08x->clk = NULL; - } else - goto out_no_clk; - } - /* Constantly enabled for now, we can control this dynamically */ - if (pl08x->clk) - clk_enable(pl08x->clk); - /* A DMA memory pool for LLIs, align on 1-byte boundary */ pl08x->pool = dma_pool_create(DRIVER_NAME, &pl08x->adev->dev, PL08X_LLI_TSFR_SIZE, PL08X_ALIGN, 0); @@ -1963,8 +1947,6 @@ out_no_irq: out_no_ioremap: dma_pool_destroy(pl08x->pool); out_no_lli_pool: - clk_put(pl08x->clk); -out_no_clk: kfree(pl08x); out_no_pl08x: amba_release_regions(adev); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 02/12] DMAENGINE: safeguard PL08X from missing platform data 2010-08-31 12:12 ` [PATCH 01/12] DMAENGINE: remove clock code from PL08X driver Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 03/12] DMAENGINE: fetch tasklet struct for PL08X include file Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij The PL08X needs some platform data with channel assignments etc to work properly, bail out of this is not supplied. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 2afb4f6..506749b 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -1819,6 +1819,13 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) goto out_no_pl08x; } + /* Get the platform data */ + pl08x->pd = dev_get_platdata(&adev->dev); + if (!pl08x->pd) { + dev_err(&adev->dev, "no platform data supplied\n"); + goto out_no_platdata; + } + /* Assign useful pointers to the driver state */ pl08x->adev = adev; pl08x->vd = vd; @@ -1879,9 +1886,6 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE"); } - /* Get the platform data */ - pl08x->pd = dev_get_platdata(&adev->dev); - /* Set caps */ dma_cap_set(DMA_MEMCPY, dmac_memcpy.cap_mask); dma_cap_set(DMA_SLAVE, dmac_slave.cap_mask); @@ -1947,6 +1951,7 @@ out_no_irq: out_no_ioremap: dma_pool_destroy(pl08x->pool); out_no_lli_pool: +out_no_platdata: kfree(pl08x); out_no_pl08x: amba_release_regions(adev); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 03/12] DMAENGINE: fetch tasklet struct for PL08X include file 2010-08-31 12:12 ` [PATCH 02/12] DMAENGINE: safeguard PL08X from missing platform data Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 04/12] DMAENGINE: use GFP_NOWAIT for PL08X allocations Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij The PL08X include file is using struct tasklet so let us bring this in apropriately. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- include/linux/amba/pl08x.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h index 8435f4a..2bb7ac4 100644 --- a/include/linux/amba/pl08x.h +++ b/include/linux/amba/pl08x.h @@ -20,6 +20,7 @@ /* We need sizes of structs from this header */ #include <linux/dmaengine.h> +#include <linux/interrupt.h> /** * struct pl08x_channel_data - data structure to pass info between -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 04/12] DMAENGINE: use GFP_NOWAIT for PL08X allocations 2010-08-31 12:12 ` [PATCH 03/12] DMAENGINE: fetch tasklet struct for PL08X include file Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 05/12] DMAENGINE: terminate transfers on PL08X mere throughly Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij Since DMA jobs can be prepared and fired in IRQ context, request GFP_NOWAIT memory for the structs. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 506749b..d1a4813 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -628,7 +628,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x, return 0; } - txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_KERNEL, + txd->llis_va = dma_pool_alloc(pl08x->pool, GFP_NOWAIT, &txd->llis_bus); if (!txd->llis_va) { dev_err(&pl08x->adev->dev, "%s no memory for llis\n", __func__); @@ -1564,7 +1564,7 @@ struct dma_async_tx_descriptor *pl08x_prep_slave_sg( dev_dbg(&pl08x->adev->dev, "%s prepare transaction of %d bytes from %s\n", __func__, sgl->length, plchan->name); - txd = kzalloc(sizeof(struct pl08x_txd), GFP_KERNEL); + txd = kzalloc(sizeof(struct pl08x_txd), GFP_NOWAIT); if (!txd) { dev_err(&pl08x->adev->dev, "%s no txd\n", __func__); return NULL; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 05/12] DMAENGINE: terminate transfers on PL08X mere throughly 2010-08-31 12:12 ` [PATCH 04/12] DMAENGINE: use GFP_NOWAIT for PL08X allocations Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 06/12] DMAENGINE: free txd list if submission fails on PL08X Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij We need to terminate pending tasklets and be sure to disable not only the channel but it's IRQ:s when we terminate all work on a PL08X channel. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index d1a4813..65f595c 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -312,6 +312,8 @@ static void pl08x_stop_phy_chan(struct pl08x_phy_chan *ch) /* Disable channel */ val = readl(ch->base + PL080_CH_CONFIG); val &= ~PL080_CONFIG_ENABLE; + val &= ~PL080_CONFIG_ERR_IRQ_MASK; + val &= ~PL080_CONFIG_TC_IRQ_MASK; writel(val, ch->base + PL080_CH_CONFIG); } @@ -1653,13 +1655,15 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, pl08x_put_phy_channel(pl08x, plchan->phychan); plchan->phychan = NULL; } - + /* Stop any pending tasklet */ + tasklet_disable(&plchan->tasklet); /* Dequeue jobs and free LLIs */ if (plchan->at) { pl08x_free_txd(pl08x, plchan->at); - pl08x_free_txd_list(pl08x, plchan); plchan->at = NULL; } + /* Dequeue jobs not yet fired as well */ + pl08x_free_txd_list(pl08x, plchan); break; case DMA_PAUSE: pl08x_pause_phy_chan(plchan->phychan); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 06/12] DMAENGINE: free txd list if submission fails on PL08X 2010-08-31 12:12 ` [PATCH 05/12] DMAENGINE: terminate transfers on PL08X mere throughly Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 07/12] DMAENGINE: add debugfs file for PL08X Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij Sometimes oversubscribed channels fail to mux in a signal line for the transmission, and we bail out. Make sure we also clean out any pending txd lists when this happens. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 65f595c..65892d5 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -1270,6 +1270,7 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx) ret = prep_phy_channel(plchan, txd); if (ret) { /* No physical channel available, cope with it */ + pl08x_free_txd_list(pl08x, plchan); spin_unlock_irqrestore(&plchan->lock, flags); return -EBUSY; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 07/12] DMAENGINE: add debugfs file for PL08X 2010-08-31 12:12 ` [PATCH 06/12] DMAENGINE: free txd list if submission fails on PL08X Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 08/12] DMAENGINE: be more explicit when freeing PL08X signals Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij This adds a debugfs file for the PL08X showing the current allocation of physical to virtual channels. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 57 insertions(+), 0 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 65892d5..a99ad19 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -68,6 +68,8 @@ #include <linux/amba/bus.h> #include <linux/dmaengine.h> #include <linux/amba/pl08x.h> +#include <linux/debugfs.h> +#include <linux/seq_file.h> #include <asm/hardware/pl080.h> #include <asm/dma.h> @@ -1806,6 +1808,60 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x, return i; } +#ifdef CONFIG_DEBUG_FS +static int pl08x_debugfs_show(struct seq_file *s, void *data) +{ + struct pl08x_driver_data *pl08x = s->private; + struct pl08x_phy_chan *ch; + unsigned long flags; + int i; + + seq_printf(s, "PL08x physical channels:\n"); + seq_printf(s, "CHANNEL:\tUSER:\n"); + seq_printf(s, "--------\t-----\n"); + for (i = 0; i < pl08x->vd->channels; i++) { + struct pl08x_dma_chan *virt_chan; + + ch = &pl08x->phy_chans[i]; + + spin_lock_irqsave(&ch->lock, flags); + virt_chan = ch->serving; + + seq_printf(s, "%d\t\t%s\n", + ch->id, virt_chan ? virt_chan->name : "(none)"); + + spin_unlock_irqrestore(&ch->lock, flags); + } + return 0; +} + +static int pl08x_debugfs_open(struct inode *inode, struct file *file) +{ + return single_open(file, pl08x_debugfs_show, inode->i_private); +} + +static const struct file_operations pl08x_debugfs_operations = { + .open = pl08x_debugfs_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +static void init_pl08x_debugfs(struct pl08x_driver_data *pl08x) +{ + /* Expose a simple debugfs interface to view all clocks */ + (void) debugfs_create_file(dev_name(&pl08x->adev->dev), S_IFREG | S_IRUGO, + NULL, pl08x, + &pl08x_debugfs_operations); + return 0; +} + +#else +static inline void init_pl08x_debugfs(struct pl08x_driver_data *pl08x) +{ +} +#endif + static int pl08x_probe(struct amba_device *adev, struct amba_id *id) { struct pl08x_driver_data *pl08x; @@ -1937,6 +1993,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) } amba_set_drvdata(adev, pl08x); + init_pl08x_debugfs(pl08x); dev_info(&pl08x->adev->dev, "ARM(R) %s DMA block initialized @%08x\n", vd->name, adev->res.start); return 0; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 08/12] DMAENGINE: be more explicit when freeing PL08X signals 2010-08-31 12:12 ` [PATCH 07/12] DMAENGINE: add debugfs file for PL08X Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 09/12] DMAENGINE: remove pointless module safeguard on PL08X Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij The signals connected to the PL08X physical channels shall not be tangled up with the physical channels themselves. This gets some reference counting right. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index a99ad19..a7ce08e 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -449,7 +449,6 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x, /* Mark it as free */ spin_lock_irqsave(&ch->lock, flags); ch->serving = NULL; - ch->signal = -1; spin_unlock_irqrestore(&ch->lock, flags); } @@ -1100,8 +1099,10 @@ static void pl08x_tasklet(unsigned long data) * No more jobs, so free up the physical channel * Free any allocated signal on slave transfers too */ - if ((phychan->signal >= 0) && pl08x->pd->put_signal) + if ((phychan->signal >= 0) && pl08x->pd->put_signal) { pl08x->pd->put_signal(plchan); + phychan->signal = -1; + } pl08x_put_phy_channel(pl08x, phychan); plchan->phychan = NULL; } @@ -1193,6 +1194,7 @@ static int prep_phy_channel(struct pl08x_dma_chan *plchan, */ if ((txd->direction == DMA_FROM_DEVICE || txd->direction == DMA_TO_DEVICE) && + ch->signal < 0 && pl08x->pd->get_signal) { ret = pl08x->pd->get_signal(plchan); if (ret < 0) { @@ -1653,8 +1655,10 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, * signal */ if ((plchan->phychan->signal >= 0) && - pl08x->pd->put_signal) + pl08x->pd->put_signal) { pl08x->pd->put_signal(plchan); + plchan->phychan->signal = -1; + } pl08x_put_phy_channel(pl08x, plchan->phychan); plchan->phychan = NULL; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 09/12] DMAENGINE: remove pointless module safeguard on PL08X 2010-08-31 12:12 ` [PATCH 08/12] DMAENGINE: be more explicit when freeing PL08X signals Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 10/12] DMAENGINE: move the PL08X to full runtime allocation Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij The PL08X driver safeguards itself from being compiled as a module for no reason: the Kconfig option is a bool anyway. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 12 ------------ 1 files changed, 0 insertions(+), 12 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index a7ce08e..f80fc4b 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -130,18 +130,6 @@ struct pl08x_driver_data { spinlock_t lock; }; -#ifdef MODULE - -# error "AMBA PL08X DMA CANNOT BE COMPILED AS A LOADABLE MODULE AT PRESENT" - -/* - a) Some devices might make use of DMA during boot - (esp true for DMAENGINE implementation) - b) Memory allocation will need much more attention - before load/unload can be supported - */ -#endif - /* * PL08X specific defines */ -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 10/12] DMAENGINE: move the PL08X to full runtime allocation 2010-08-31 12:12 ` [PATCH 09/12] DMAENGINE: remove pointless module safeguard on PL08X Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij This change makes the PL08X not being a singleton anymore, this piece was missing earlier: we move the struct dma_device entries into the driver state holder. Also flag channels as slaves and fix up the channel freeing code. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 112 +++++++++++++++++++++++++------------------- include/linux/amba/pl08x.h | 2 + 2 files changed, 66 insertions(+), 48 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index f80fc4b..4573189 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -110,6 +110,8 @@ struct lli { /** * struct pl08x_driver_data - the local state holder for the PL08x + * @slave: slave engine for this instance + * @memcpy: memcpy engine for this instance * @base: virtual memory base (remapped) for the PL08x * @adev: the corresponding AMBA (PrimeCell) bus entry * @vd: vendor data for this PL08x variant @@ -120,6 +122,8 @@ struct lli { * @lock: a spinlock for this struct */ struct pl08x_driver_data { + struct dma_device slave; + struct dma_device memcpy; void __iomem *base; struct amba_device *adev; struct vendor_data *vd; @@ -1691,38 +1695,6 @@ bool pl08x_filter_id(struct dma_chan *chan, void *chan_id) return false; } -static struct dma_device dmac_memcpy = { - .device_alloc_chan_resources = pl08x_alloc_chan_resources, - .device_free_chan_resources = pl08x_free_chan_resources, - .device_prep_dma_memcpy = pl08x_prep_dma_memcpy, - .device_prep_dma_xor = NULL, - .device_prep_dma_memset = NULL, - .device_prep_dma_interrupt = pl08x_prep_dma_interrupt, - .device_tx_status = pl08x_dma_tx_status, - .device_issue_pending = pl08x_issue_pending, - .device_control = pl08x_control, - /* - * Align to 4-byte boundary - * This makes the DMAtests fail with grace on PB1176 - * broken DMA hardware instead of locking everything - * up. - */ - /* .copy_align = 2, */ -}; - -static struct dma_device dmac_slave = { - .device_alloc_chan_resources = pl08x_alloc_chan_resources, - .device_free_chan_resources = pl08x_free_chan_resources, - .device_prep_dma_xor = NULL, - .device_prep_dma_memset = NULL, - .device_prep_dma_interrupt = pl08x_prep_dma_interrupt, - .device_tx_status = pl08x_dma_tx_status, - .device_issue_pending = pl08x_issue_pending, - .device_prep_slave_sg = pl08x_prep_slave_sg, - .device_control = pl08x_control, -}; - - /* * Just check that the device is there and active * TODO: turn this bit on/off depending on the number of @@ -1770,6 +1742,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x, chan->host = pl08x; if (slave) { + chan->slave = true; chan->name = pl08x->pd->slave_channels[i].bus_id; chan->cd = &pl08x->pd->slave_channels[i]; } else { @@ -1800,10 +1773,23 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x, return i; } +static void pl08x_free_virtual_channels(struct dma_device *dmadev) +{ + struct pl08x_dma_chan *chan = NULL; + struct pl08x_dma_chan *next; + + list_for_each_entry_safe(chan, + next, &dmadev->channels, chan.device_node) { + list_del(&chan->chan.device_node); + kfree(chan); + } +} + #ifdef CONFIG_DEBUG_FS static int pl08x_debugfs_show(struct seq_file *s, void *data) { struct pl08x_driver_data *pl08x = s->private; + struct pl08x_dma_chan *chan; struct pl08x_phy_chan *ch; unsigned long flags; int i; @@ -1824,6 +1810,21 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data) spin_unlock_irqrestore(&ch->lock, flags); } + + seq_printf(s, "\nPL08x virtual memcpy channels:\n"); + seq_printf(s, "CHANNEL:\n"); + seq_printf(s, "--------\n"); + list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) { + seq_printf(s, "%s\n", chan->name); + } + + seq_printf(s, "\nPL08x virtual slave channels:\n"); + seq_printf(s, "CHANNEL:\n"); + seq_printf(s, "--------\n"); + list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) { + seq_printf(s, "%s\n", chan->name); + } + return 0; } @@ -1845,7 +1846,6 @@ static void init_pl08x_debugfs(struct pl08x_driver_data *pl08x) (void) debugfs_create_file(dev_name(&pl08x->adev->dev), S_IFREG | S_IRUGO, NULL, pl08x, &pl08x_debugfs_operations); - return 0; } #else @@ -1872,6 +1872,28 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) goto out_no_pl08x; } + /* Initialize memcpy engine */ + dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask); + pl08x->memcpy.dev = &adev->dev; + pl08x->memcpy.device_alloc_chan_resources = pl08x_alloc_chan_resources; + pl08x->memcpy.device_free_chan_resources = pl08x_free_chan_resources; + pl08x->memcpy.device_prep_dma_memcpy = pl08x_prep_dma_memcpy; + pl08x->memcpy.device_prep_dma_interrupt = pl08x_prep_dma_interrupt; + pl08x->memcpy.device_tx_status = pl08x_dma_tx_status; + pl08x->memcpy.device_issue_pending = pl08x_issue_pending; + pl08x->memcpy.device_control = pl08x_control; + + /* Initialize slave engine */ + dma_cap_set(DMA_SLAVE, pl08x->slave.cap_mask); + pl08x->slave.dev = &adev->dev; + pl08x->slave.device_alloc_chan_resources = pl08x_alloc_chan_resources; + pl08x->slave.device_free_chan_resources = pl08x_free_chan_resources; + pl08x->slave.device_prep_dma_interrupt = pl08x_prep_dma_interrupt; + pl08x->slave.device_tx_status = pl08x_dma_tx_status; + pl08x->slave.device_issue_pending = pl08x_issue_pending; + pl08x->slave.device_prep_slave_sg = pl08x_prep_slave_sg; + pl08x->slave.device_control = pl08x_control; + /* Get the platform data */ pl08x->pd = dev_get_platdata(&adev->dev); if (!pl08x->pd) { @@ -1939,14 +1961,8 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) pl08x_phy_channel_busy(ch) ? "BUSY" : "FREE"); } - /* Set caps */ - dma_cap_set(DMA_MEMCPY, dmac_memcpy.cap_mask); - dma_cap_set(DMA_SLAVE, dmac_slave.cap_mask); - dmac_memcpy.dev = &adev->dev; - dmac_slave.dev = &adev->dev; - /* Register as many memcpy channels as there are physical channels */ - ret = pl08x_dma_init_virtual_channels(pl08x, &dmac_memcpy, + ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->memcpy, pl08x->vd->channels, false); if (ret <= 0) { dev_warn(&pl08x->adev->dev, @@ -1954,10 +1970,10 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) __func__, ret); goto out_no_memcpy; } - dmac_memcpy.chancnt = ret; + pl08x->memcpy.chancnt = ret; /* Register slave channels */ - ret = pl08x_dma_init_virtual_channels(pl08x, &dmac_slave, + ret = pl08x_dma_init_virtual_channels(pl08x, &pl08x->slave, pl08x->pd->num_slave_channels, true); if (ret <= 0) { @@ -1966,9 +1982,9 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) __func__, ret); goto out_no_slave; } - dmac_slave.chancnt = ret; + pl08x->slave.chancnt = ret; - ret = dma_async_device_register(&dmac_memcpy); + ret = dma_async_device_register(&pl08x->memcpy); if (ret) { dev_warn(&pl08x->adev->dev, "%s failed to register memcpy as an async device - %d\n", @@ -1976,7 +1992,7 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) goto out_no_memcpy_reg; } - ret = dma_async_device_register(&dmac_slave); + ret = dma_async_device_register(&pl08x->slave); if (ret) { dev_warn(&pl08x->adev->dev, "%s failed to register slave as an async device - %d\n", @@ -1991,11 +2007,11 @@ static int pl08x_probe(struct amba_device *adev, struct amba_id *id) return 0; out_no_slave_reg: - dma_async_device_unregister(&dmac_memcpy); + dma_async_device_unregister(&pl08x->memcpy); out_no_memcpy_reg: - /* FIXME: free slave channels */ + pl08x_free_virtual_channels(&pl08x->slave); out_no_slave: - /* FIXME: free memcpy channels */ + pl08x_free_virtual_channels(&pl08x->memcpy); out_no_memcpy: kfree(pl08x->phy_chans); out_no_phychans: diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h index 2bb7ac4..6db44f9 100644 --- a/include/linux/amba/pl08x.h +++ b/include/linux/amba/pl08x.h @@ -138,6 +138,7 @@ struct pl08x_txd { * @lock: a lock for this channel data * @host: a pointer to the host (internal use) * @paused: whether the channel is paused + * @slave: whether this channel is a device (slave) or for memcpy */ struct pl08x_dma_chan { struct dma_chan chan; @@ -154,6 +155,7 @@ struct pl08x_dma_chan { spinlock_t lock; void *host; bool paused; + bool slave; }; /** -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-08-31 12:12 ` [PATCH 10/12] DMAENGINE: move the PL08X to full runtime allocation Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-08-31 12:12 ` [PATCH 12/12] DMAENGINE: define channel states for the PL08X Linus Walleij 2010-09-23 4:59 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Dan Williams 0 siblings, 2 replies; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij This change makes the memcpy transfers wait for a physical channel to become available if no free channel is available when the job is submitted. When the first physical channel fires its tasklet, it will spin over the memcpy channels to see if one of these is waiting. This is necessary to get the memcpy semantics right: the generic memcpy API assume transfers never fail, and with our oversubscribed physical channels this becomes a problem: sometimes submit would fail. This fixes it by letting the memcpy channels pull a free channel ASAP. The slave channels shall however *fail* if no channel is available since the device will then either fall back to some PIO mode or retry. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 299 ++++++++++++++++++++++++-------------------- include/linux/amba/pl08x.h | 3 + 2 files changed, 168 insertions(+), 134 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 4573189..49fb19d 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -1018,134 +1018,6 @@ static void pl08x_free_txd_list(struct pl08x_driver_data *pl08x, } } -static void pl08x_tasklet(unsigned long data) -{ - struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data; - struct pl08x_phy_chan *phychan = plchan->phychan; - struct pl08x_driver_data *pl08x = plchan->host; - unsigned long flags; - - if (!plchan) - BUG(); - - spin_lock_irqsave(&plchan->lock, flags); - - if (plchan->at) { - dma_async_tx_callback callback = - plchan->at->tx.callback; - void *callback_param = - plchan->at->tx.callback_param; - - /* - * Update last completed - */ - plchan->lc = - (plchan->at->tx.cookie); - - /* - * Callback to signal completion - */ - if (callback) - callback(callback_param); - - /* - * Device callbacks should NOT clear - * the current transaction on the channel - * Linus: sometimes they should? - */ - if (!plchan->at) - BUG(); - - /* - * Free the descriptor if it's not for a device - * using a circular buffer - */ - if (!plchan->at->cd->circular_buffer) { - pl08x_free_txd(pl08x, plchan->at); - plchan->at = NULL; - } - /* - * else descriptor for circular - * buffers only freed when - * client has disabled dma - */ - } - /* - * If a new descriptor is queued, set it up - * plchan->at is NULL here - */ - if (!list_empty(&plchan->desc_list)) { - struct pl08x_txd *next; - - next = list_first_entry(&plchan->desc_list, - struct pl08x_txd, - node); - list_del(&next->node); - plchan->at = next; - /* Configure the physical channel for the next txd */ - pl08x_config_phychan_for_txd(plchan); - pl08x_set_cregs(pl08x, plchan->phychan); - pl08x_enable_phy_chan(pl08x, plchan->phychan); - } else { - /* - * No more jobs, so free up the physical channel - * Free any allocated signal on slave transfers too - */ - if ((phychan->signal >= 0) && pl08x->pd->put_signal) { - pl08x->pd->put_signal(plchan); - phychan->signal = -1; - } - pl08x_put_phy_channel(pl08x, phychan); - plchan->phychan = NULL; - } - - spin_unlock_irqrestore(&plchan->lock, flags); -} - -static irqreturn_t pl08x_irq(int irq, void *dev) -{ - struct pl08x_driver_data *pl08x = dev; - u32 mask = 0; - u32 val; - int i; - - val = readl(pl08x->base + PL080_ERR_STATUS); - if (val) { - /* - * An error interrupt (on one or more channels) - */ - dev_err(&pl08x->adev->dev, - "%s error interrupt, register value 0x%08x\n", - __func__, val); - /* - * Simply clear ALL PL08X error interrupts, - * regardless of channel and cause - * FIXME: should be 0x00000003 on PL081 really. - */ - writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR); - } - val = readl(pl08x->base + PL080_INT_STATUS); - for (i = 0; i < pl08x->vd->channels; i++) { - if ((1 << i) & val) { - /* Locate physical channel */ - struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i]; - struct pl08x_dma_chan *plchan = phychan->serving; - - /* Schedule tasklet on this channel */ - tasklet_schedule(&plchan->tasklet); - - mask |= (1 << i); - } - } - /* - * Clear only the terminal interrupts on channels we processed - */ - writel(mask, pl08x->base + PL080_TC_CLEAR); - - return mask ? IRQ_HANDLED : IRQ_NONE; -} - - /* * The DMA ENGINE API */ @@ -1265,12 +1137,20 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx) */ ret = prep_phy_channel(plchan, txd); if (ret) { - /* No physical channel available, cope with it */ - pl08x_free_txd_list(pl08x, plchan); - spin_unlock_irqrestore(&plchan->lock, flags); - return -EBUSY; + /* + * No physical channel available, we will + * stack up the memcpy channels until there is a channel + * available to handle it whereas slave transfers cannot + * wait and will simply be NACK:ed with -EBUSY. + */ + if (plchan->slave) { + /* No physical channel available, cope with it */ + pl08x_free_txd_list(pl08x, plchan); + spin_unlock_irqrestore(&plchan->lock, flags); + return -EBUSY; + } else + plchan->waiting = txd; } - spin_unlock_irqrestore(&plchan->lock, flags); return tx->cookie; @@ -1478,7 +1358,6 @@ static void pl08x_issue_pending(struct dma_chan *chan) struct pl08x_driver_data *pl08x = plchan->host; unsigned long flags; - spin_lock_irqsave(&plchan->lock, flags); /* Something is already active */ if (plchan->at) { @@ -1486,6 +1365,10 @@ static void pl08x_issue_pending(struct dma_chan *chan) return; } + /* Didn't get a physical channel so waiting for it ... */ + if (plchan->waiting) + return; + /* Take the first element in the queue and execute it */ if (!list_empty(&plchan->desc_list)) { struct pl08x_txd *next; @@ -1713,6 +1596,154 @@ static void pl08x_ensure_on(struct pl08x_driver_data *pl08x) writel(val, pl08x->base + PL080_CONFIG); } +static void pl08x_tasklet(unsigned long data) +{ + struct pl08x_dma_chan *plchan = (struct pl08x_dma_chan *) data; + struct pl08x_phy_chan *phychan = plchan->phychan; + struct pl08x_driver_data *pl08x = plchan->host; + + if (!plchan) + BUG(); + + spin_lock(&plchan->lock); + + if (plchan->at) { + dma_async_tx_callback callback = + plchan->at->tx.callback; + void *callback_param = + plchan->at->tx.callback_param; + + /* + * Update last completed + */ + plchan->lc = + (plchan->at->tx.cookie); + + /* + * Callback to signal completion + */ + if (callback) + callback(callback_param); + + /* + * Device callbacks should NOT clear + * the current transaction on the channel + * Linus: sometimes they should? + */ + if (!plchan->at) + BUG(); + + /* + * Free the descriptor if it's not for a device + * using a circular buffer + */ + if (!plchan->at->cd->circular_buffer) { + pl08x_free_txd(pl08x, plchan->at); + plchan->at = NULL; + } + /* + * else descriptor for circular + * buffers only freed when + * client has disabled dma + */ + } + /* + * If a new descriptor is queued, set it up + * plchan->at is NULL here + */ + if (!list_empty(&plchan->desc_list)) { + struct pl08x_txd *next; + + next = list_first_entry(&plchan->desc_list, + struct pl08x_txd, + node); + list_del(&next->node); + plchan->at = next; + /* Configure the physical channel for the next txd */ + pl08x_config_phychan_for_txd(plchan); + pl08x_set_cregs(pl08x, plchan->phychan); + pl08x_enable_phy_chan(pl08x, plchan->phychan); + } else { + struct pl08x_dma_chan *waiting = NULL; + + /* + * No more jobs, so free up the physical channel + * Free any allocated signal on slave transfers too + */ + if ((phychan->signal >= 0) && pl08x->pd->put_signal) { + pl08x->pd->put_signal(plchan); + phychan->signal = -1; + } + pl08x_put_phy_channel(pl08x, phychan); + plchan->phychan = NULL; + + /* + * And NOW before anyone else can grab that free:d + * up physical channel, see if there is some memcpy + * pending that seriously needs to start because of + * being stacked up while we were choking the + * physical channels with data. + */ + list_for_each_entry(waiting, &pl08x->memcpy.channels, chan.device_node) { + if (waiting->waiting) { + int ret; + + /* This should REALLY not fail now */ + ret = prep_phy_channel(waiting, waiting->waiting); + BUG_ON(ret); + waiting->waiting = NULL; + pl08x_issue_pending(&waiting->chan); + break; + } + } + } + + spin_unlock(&plchan->lock); +} + +static irqreturn_t pl08x_irq(int irq, void *dev) +{ + struct pl08x_driver_data *pl08x = dev; + u32 mask = 0; + u32 val; + int i; + + val = readl(pl08x->base + PL080_ERR_STATUS); + if (val) { + /* + * An error interrupt (on one or more channels) + */ + dev_err(&pl08x->adev->dev, + "%s error interrupt, register value 0x%08x\n", + __func__, val); + /* + * Simply clear ALL PL08X error interrupts, + * regardless of channel and cause + * FIXME: should be 0x00000003 on PL081 really. + */ + writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR); + } + val = readl(pl08x->base + PL080_INT_STATUS); + for (i = 0; i < pl08x->vd->channels; i++) { + if ((1 << i) & val) { + /* Locate physical channel */ + struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i]; + struct pl08x_dma_chan *plchan = phychan->serving; + + /* Schedule tasklet on this channel */ + tasklet_schedule(&plchan->tasklet); + + mask |= (1 << i); + } + } + /* + * Clear only the terminal interrupts on channels we processed + */ + writel(mask, pl08x->base + PL080_TC_CLEAR); + + return mask ? IRQ_HANDLED : IRQ_NONE; +} + /* * Initialise the DMAC memcpy/slave channels. * Make a local wrapper to hold required data diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h index 6db44f9..f461648 100644 --- a/include/linux/amba/pl08x.h +++ b/include/linux/amba/pl08x.h @@ -139,6 +139,8 @@ struct pl08x_txd { * @host: a pointer to the host (internal use) * @paused: whether the channel is paused * @slave: whether this channel is a device (slave) or for memcpy + * @waiting: a TX descriptor on this channel which is waiting for + * a physical channel to become available */ struct pl08x_dma_chan { struct dma_chan chan; @@ -156,6 +158,7 @@ struct pl08x_dma_chan { void *host; bool paused; bool slave; + struct pl08x_txd *waiting; }; /** -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 12/12] DMAENGINE: define channel states for the PL08X 2010-08-31 12:12 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Linus Walleij @ 2010-08-31 12:12 ` Linus Walleij 2010-09-23 4:59 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Dan Williams 1 sibling, 0 replies; 20+ messages in thread From: Linus Walleij @ 2010-08-31 12:12 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel, Linus Walleij Instead of strange things like a bool indicating paused state for channels, let's define a proper channel state and use that. Also print it out in the debugfs so we get a nice overview of the channels and states. Signed-off-by: Linus Walleij <linus.walleij@stericsson.com> --- drivers/dma/amba-pl08x.c | 70 ++++++++++++++++++++++++++++++++++---------- include/linux/amba/pl08x.h | 22 ++++++++++++- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 49fb19d..1718b2e 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -1148,9 +1148,20 @@ static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx) pl08x_free_txd_list(pl08x, plchan); spin_unlock_irqrestore(&plchan->lock, flags); return -EBUSY; - } else + } else { + plchan->state = PL08X_CHAN_WAITING; plchan->waiting = txd; - } + } + } else + /* + * Else we're all set, paused and ready to roll, + * status will switch to PL08X_CHAN_RUNNING when + * we call issue_pending(). If there is something + * running on the channel already we don't change + * its state. + */ + if (plchan->state == PL08X_CHAN_IDLE) + plchan->state = PL08X_CHAN_PAUSED; spin_unlock_irqrestore(&plchan->lock, flags); return tx->cookie; @@ -1206,9 +1217,10 @@ pl08x_dma_tx_status(struct dma_chan *chan, dma_set_tx_state(txstate, last_complete, last_used, bytesleft); - if (plchan->paused) + if (plchan->state == PL08X_CHAN_PAUSED) return DMA_PAUSED; + /* Whether waiting or running, we're in progress */ return DMA_IN_PROGRESS; } @@ -1366,7 +1378,7 @@ static void pl08x_issue_pending(struct dma_chan *chan) } /* Didn't get a physical channel so waiting for it ... */ - if (plchan->waiting) + if (plchan->state == PL08X_CHAN_WAITING) return; /* Take the first element in the queue and execute it */ @@ -1378,6 +1390,7 @@ static void pl08x_issue_pending(struct dma_chan *chan) node); list_del(&next->node); plchan->at = next; + plchan->state = PL08X_CHAN_RUNNING; /* Configure the physical channel for the active txd */ pl08x_config_phychan_for_txd(plchan); @@ -1520,7 +1533,7 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, switch (cmd) { case DMA_TERMINATE_ALL: - plchan->paused = false; + plchan->state = PL08X_CHAN_IDLE; if (plchan->phychan) { pl08x_stop_phy_chan(plchan->phychan); @@ -1549,11 +1562,11 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, break; case DMA_PAUSE: pl08x_pause_phy_chan(plchan->phychan); - plchan->paused = true; + plchan->state = PL08X_CHAN_PAUSED; break; case DMA_RESUME: pl08x_resume_phy_chan(plchan->phychan); - plchan->paused = false; + plchan->state = PL08X_CHAN_RUNNING; break; default: /* Unknown command */ @@ -1676,6 +1689,7 @@ static void pl08x_tasklet(unsigned long data) } pl08x_put_phy_channel(pl08x, phychan); plchan->phychan = NULL; + plchan->state = PL08X_CHAN_IDLE; /* * And NOW before anyone else can grab that free:d @@ -1684,13 +1698,17 @@ static void pl08x_tasklet(unsigned long data) * being stacked up while we were choking the * physical channels with data. */ - list_for_each_entry(waiting, &pl08x->memcpy.channels, chan.device_node) { - if (waiting->waiting) { + list_for_each_entry(waiting, &pl08x->memcpy.channels, + chan.device_node) { + if (waiting->state == PL08X_CHAN_WAITING && + waiting->waiting != NULL) { int ret; /* This should REALLY not fail now */ - ret = prep_phy_channel(waiting, waiting->waiting); + ret = prep_phy_channel(waiting, + waiting->waiting); BUG_ON(ret); + waiting->state = PL08X_CHAN_RUNNING; waiting->waiting = NULL; pl08x_issue_pending(&waiting->chan); break; @@ -1771,6 +1789,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x, } chan->host = pl08x; + chan->state = PL08X_CHAN_IDLE; if (slave) { chan->slave = true; @@ -1817,6 +1836,23 @@ static void pl08x_free_virtual_channels(struct dma_device *dmadev) } #ifdef CONFIG_DEBUG_FS +static const char *pl08x_state_str(enum pl08x_dma_chan_state state) +{ + switch(state) { + case PL08X_CHAN_IDLE: + return "idle"; + case PL08X_CHAN_RUNNING: + return "running"; + case PL08X_CHAN_PAUSED: + return "paused"; + case PL08X_CHAN_WAITING: + return "waiting"; + default: + break; + } + return "UNKNOWN STATE"; +} + static int pl08x_debugfs_show(struct seq_file *s, void *data) { struct pl08x_driver_data *pl08x = s->private; @@ -1843,17 +1879,19 @@ static int pl08x_debugfs_show(struct seq_file *s, void *data) } seq_printf(s, "\nPL08x virtual memcpy channels:\n"); - seq_printf(s, "CHANNEL:\n"); - seq_printf(s, "--------\n"); + seq_printf(s, "CHANNEL:\tSTATE:\n"); + seq_printf(s, "--------\t------\n"); list_for_each_entry(chan, &pl08x->memcpy.channels, chan.device_node) { - seq_printf(s, "%s\n", chan->name); + seq_printf(s, "%s\t\t\%s\n", chan->name, + pl08x_state_str(chan->state)); } seq_printf(s, "\nPL08x virtual slave channels:\n"); - seq_printf(s, "CHANNEL:\n"); - seq_printf(s, "--------\n"); + seq_printf(s, "CHANNEL:\tSTATE:\n"); + seq_printf(s, "--------\t------\n"); list_for_each_entry(chan, &pl08x->slave.channels, chan.device_node) { - seq_printf(s, "%s\n", chan->name); + seq_printf(s, "%s\t\t\%s\n", chan->name, + pl08x_state_str(chan->state)); } return 0; diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h index f461648..d54fbff 100644 --- a/include/linux/amba/pl08x.h +++ b/include/linux/amba/pl08x.h @@ -123,6 +123,24 @@ struct pl08x_txd { }; /** + * struct pl08x_dma_chan_state - holds the PL08x specific virtual + * channel states + * @PL08X_CHAN_IDLE: the channel is idle + * @PL08X_CHAN_RUNNING: the channel has allocated a physical transport + * channel and is running a transfer on it + * @PL08X_CHAN_PAUSED: the channel has allocated a physical transport + * channel, but the transfer is currently paused + * @PL08X_CHAN_WAITING: the channel is waiting for a physical transport + * channel to become available (only pertains to memcpy channels) + */ +enum pl08x_dma_chan_state { + PL08X_CHAN_IDLE, + PL08X_CHAN_RUNNING, + PL08X_CHAN_PAUSED, + PL08X_CHAN_WAITING, +}; + +/** * struct pl08x_dma_chan - this structure wraps a DMA ENGINE channel * @chan: wrappped abstract channel * @phychan: the physical channel utilized by this channel, if there is one @@ -137,7 +155,7 @@ struct pl08x_txd { * @at: active transaction on this channel * @lock: a lock for this channel data * @host: a pointer to the host (internal use) - * @paused: whether the channel is paused + * @state: whether the channel is idle, paused, running etc * @slave: whether this channel is a device (slave) or for memcpy * @waiting: a TX descriptor on this channel which is waiting for * a physical channel to become available @@ -156,7 +174,7 @@ struct pl08x_dma_chan { struct pl08x_txd *at; spinlock_t lock; void *host; - bool paused; + enum pl08x_dma_chan_state state; bool slave; struct pl08x_txd *waiting; }; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-08-31 12:12 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Linus Walleij 2010-08-31 12:12 ` [PATCH 12/12] DMAENGINE: define channel states for the PL08X Linus Walleij @ 2010-09-23 4:59 ` Dan Williams 2010-09-23 7:39 ` Linus Walleij 2010-09-23 7:46 ` Linus Walleij 1 sibling, 2 replies; 20+ messages in thread From: Dan Williams @ 2010-09-23 4:59 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-arm-kernel, yuanyabin1978, linux-kernel On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij <linus.walleij@stericsson.com> wrote: > This change makes the memcpy transfers wait for a physical channel > to become available if no free channel is available when the job > is submitted. When the first physical channel fires its tasklet, > it will spin over the memcpy channels to see if one of these is > waiting. > > This is necessary to get the memcpy semantics right: the generic > memcpy API assume transfers never fail, and with our oversubscribed > physical channels this becomes a problem: sometimes submit would > fail. This fixes it by letting the memcpy channels pull a free > channel ASAP. > > The slave channels shall however *fail* if no channel is available > since the device will then either fall back to some PIO mode or > retry. > This patch does not sit right with me. It seems a bit arbitrary that memcpy operations will be queued while slave operations are failed. Is there anyway to know at prep time whether a subsequent submit will fail? Are there any cases where a slave might want its operation queued? The prep routine is meant to guarantee that all the resources for a transaction have been acquired. The only reason ->tx_submit() has a return value is to support the net_dma usage model that uses opaque cookies for tracking transactions. If we make tx_submit() fallable we should go back and ensure that all usages are prepared to handle failure. -- Dan sidenote: this driver needs to be converted to a dma descriptor pool (allocated at device_alloc_chan_resources time), or at a minimum use GFP_NOWAIT in device_prep_dma_memcpy because that routine may be called from atomic contexts. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-09-23 4:59 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Dan Williams @ 2010-09-23 7:39 ` Linus Walleij 2010-09-23 15:29 ` Dan Williams 2010-09-23 7:46 ` Linus Walleij 1 sibling, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-09-23 7:39 UTC (permalink / raw) To: Dan Williams Cc: linux-arm-kernel, yuanyabin1978, linux-kernel, Per Fridén, Rabin VINCENT 2010/9/23 Dan Williams <dan.j.williams@intel.com>: > On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij > <linus.walleij@stericsson.com> wrote: >> This change makes the memcpy transfers wait for a physical channel >> to become available if no free channel is available when the job >> is submitted. When the first physical channel fires its tasklet, >> it will spin over the memcpy channels to see if one of these is >> waiting. >> >> This is necessary to get the memcpy semantics right: the generic >> memcpy API assume transfers never fail, and with our oversubscribed >> physical channels this becomes a problem: sometimes submit would >> fail. This fixes it by letting the memcpy channels pull a free >> channel ASAP. >> >> The slave channels shall however *fail* if no channel is available >> since the device will then either fall back to some PIO mode or >> retry. >> > > This patch does not sit right with me. It seems a bit arbitrary that > memcpy operations will be queued while slave operations are failed. I was sort of suspecting a discussion around this... Background: the PL081 that I'm testing on only has *two* physical channels, they can be used for memcpy, slave transfers (RX/TX). My first option was to set one channel aside for memcpy and one for dev xfers and be done with it. But that would be devastating if I was only using them for memcpy or only devxfer, since it would slash performance in half. So I decided to let memcpy and dev xfers battle for the channels with oversubscription and exposing two virtual memcpy channels. Whereas the slave drivers I've written are prepared to handle the case where a transfer fails gracefully (and not treat it as an error) and fall back to IRQ/PIO mode, the memcpy tests in drivers/dma/dmatest.c does treat a failure to prep() or submit() as an error. So for this reason I'm queueing memcpy requests until there is a channel ready. Are you suggesting I should rewrite the memcpy tests instead so they gracefully handle a failed prep() call, not treat it as an error and either: - retry or - copy with a regular klibc memcpy() call? > Is there anyway to know at prep time whether a subsequent submit will > fail? Are there any cases where a slave might want its operation > queued? > > The prep routine is meant to guarantee that all the resources for a > transaction have been acquired. The only reason ->tx_submit() has a > return value is to support the net_dma usage model that uses opaque > cookies for tracking transactions. It is possible to do this at prep() time. However that assumes that every device transfer has code that executes this sequence: .device_prep_slave_sg() .tx_submit() .device_issue_pending() In direct succession. If there is an arbitrary delay between prep() and submit() (where I currently fail xfers) the physical channels may starve if prep() allocates them. If I can safely assume that prep() and .tx_submit() are in quick succession, I can reserve the physical channel at prep() time. This seems to be the case in current code where only quick things like setting a callback etc is done between prep() and .tx_submit(). So I'll spin the PL08x around to reserve channels on prep() instead. (By the way: we should rename .tx_submit() to just .submit() since in device context this can just as well be RX!) > If we make tx_submit() fallable we should go back and ensure that all > usages are prepared to handle failure. OK I've implemented failure path for tx_submit() in all my drivers but I also have it for prep() of course. I hope I can keep that code... I'll take out the comments about allocation failure and move them to the prep() calls though. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-09-23 7:39 ` Linus Walleij @ 2010-09-23 15:29 ` Dan Williams 0 siblings, 0 replies; 20+ messages in thread From: Dan Williams @ 2010-09-23 15:29 UTC (permalink / raw) To: Linus Walleij Cc: linux-arm-kernel, yuanyabin1978, linux-kernel, Per Fridén, Rabin VINCENT On Thu, Sep 23, 2010 at 12:39 AM, Linus Walleij <linus.ml.walleij@gmail.com> wrote: > 2010/9/23 Dan Williams <dan.j.williams@intel.com>: >> On Tue, Aug 31, 2010 at 5:12 AM, Linus Walleij >> <linus.walleij@stericsson.com> wrote: >>> This change makes the memcpy transfers wait for a physical channel >>> to become available if no free channel is available when the job >>> is submitted. When the first physical channel fires its tasklet, >>> it will spin over the memcpy channels to see if one of these is >>> waiting. >>> >>> This is necessary to get the memcpy semantics right: the generic >>> memcpy API assume transfers never fail, and with our oversubscribed >>> physical channels this becomes a problem: sometimes submit would >>> fail. This fixes it by letting the memcpy channels pull a free >>> channel ASAP. >>> >>> The slave channels shall however *fail* if no channel is available >>> since the device will then either fall back to some PIO mode or >>> retry. >>> >> >> This patch does not sit right with me. It seems a bit arbitrary that >> memcpy operations will be queued while slave operations are failed. > > I was sort of suspecting a discussion around this... > > Background: the PL081 that I'm testing on only has *two* physical > channels, they can be used for memcpy, slave transfers (RX/TX). > > My first option was to set one channel aside for memcpy and > one for dev xfers and be done with it. > > But that would be devastating if > I was only using them for memcpy or only devxfer, since it would > slash performance in half. So I decided to let memcpy and > dev xfers battle for the channels with oversubscription and exposing > two virtual memcpy channels. > > Whereas the slave drivers I've written are prepared to handle the > case where a transfer fails gracefully (and not treat it as an error) > and fall back to IRQ/PIO mode, the memcpy tests in > drivers/dma/dmatest.c does treat a failure to prep() or submit() > as an error. Yeah, that is a hold over from when the original ioatdma driver performed memory allocations at submit time (fixed here a0587bcf). > > So for this reason I'm queueing memcpy requests until > there is a channel ready. > > Are you suggesting I should rewrite the memcpy tests instead > so they gracefully handle a failed prep() call, not treat it as an > error and either: > > - retry or > - copy with a regular klibc memcpy() call? I was more thinking that at a minimum all slave drivers would follow the same semantic and expect their submit requests to be queued or fail, as it stands we have a mix of both across all slave implementations. > >> Is there anyway to know at prep time whether a subsequent submit will >> fail? Are there any cases where a slave might want its operation >> queued? >> >> The prep routine is meant to guarantee that all the resources for a >> transaction have been acquired. The only reason ->tx_submit() has a >> return value is to support the net_dma usage model that uses opaque >> cookies for tracking transactions. > > It is possible to do this at prep() time. However that assumes that every > device transfer has code that executes this sequence: > > .device_prep_slave_sg() > .tx_submit() > .device_issue_pending() > > In direct succession. If there is an arbitrary delay between prep() > and submit() (where I currently fail xfers) the physical channels > may starve if prep() allocates them. There is precedent, albeit a bit inelegant, for taking a lock in prep() and dropping it in submit(). The ioatdma driver does this to ensure in-order submission because the hardware caches the address of the next descriptor in the ring. Out-of-order submission would require a hardware reset to clear that cache prep() and submit() are not paired. > If I can safely assume that prep() and .tx_submit() are in quick > succession, I can reserve the physical channel at prep() time. > It is safe to actively enforce this as long as you never plan to support the async_tx channel switching capability (only a few raid drivers use this presently). I'm converting to be an explicit opt-in because most drivers don't need this and can use the cut down version of dma_async_tx_descriptor. > This seems to be the case in current code where only quick > things like setting a callback etc is done between prep() and > .tx_submit(). > > So I'll spin the PL08x around to reserve channels on > prep() instead. > > (By the way: we should rename .tx_submit() to just .submit() > since in device context this can just as well be RX!) The 'tx' in async_tx came "asynchronous transfer/transform" where the 'x' is transfer or transform. I'd also like to change drop the 'device_' from the method names and rename dma_async_tx_descriptor to something shorter, but I'm not sure it is worth the thrash. >> If we make tx_submit() fallable we should go back and ensure that all >> usages are prepared to handle failure. > > OK I've implemented failure path for tx_submit() in all my > drivers but I also have it for prep() of course. I hope I can keep > that code... I'll take out the comments about allocation failure > and move them to the prep() calls though. > Sounds good. -- Dan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-09-23 4:59 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Dan Williams 2010-09-23 7:39 ` Linus Walleij @ 2010-09-23 7:46 ` Linus Walleij 2010-09-23 15:35 ` Dan Williams 1 sibling, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-09-23 7:46 UTC (permalink / raw) To: Dan Williams Cc: linux-arm-kernel, yuanyabin1978, linux-kernel, Per Fridén, Rabin VINCENT 2010/9/23 Dan Williams <dan.j.williams@intel.com>: > The prep routine is meant to guarantee that all the resources for a > transaction have been acquired. A practicality here: The assumption to fail for physical channel allocation at .tx_submit() is already in the first patch adding the driver. Should I squash all the icremental improvements into the original driver, convert to fail at .prep() and resubmit to make all of this easier, or do you want me to refactor the patch stack, or refactor from patch 11/12 or...? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait 2010-09-23 7:46 ` Linus Walleij @ 2010-09-23 15:35 ` Dan Williams 0 siblings, 0 replies; 20+ messages in thread From: Dan Williams @ 2010-09-23 15:35 UTC (permalink / raw) To: Linus Walleij Cc: linux-arm-kernel, yuanyabin1978, linux-kernel, Per Fridén, Rabin VINCENT On Thu, Sep 23, 2010 at 12:46 AM, Linus Walleij <linus.ml.walleij@gmail.com> wrote: > 2010/9/23 Dan Williams <dan.j.williams@intel.com>: > >> The prep routine is meant to guarantee that all the resources for a >> transaction have been acquired. > > A practicality here: > > The assumption to fail for physical channel allocation at .tx_submit() > is already in the first patch adding the driver. > > Should I squash all the icremental improvements into the original > driver, convert to fail at .prep() and resubmit to make all of this > easier, or do you want me to refactor the patch stack, or refactor > from patch 11/12 or...? > At this point I am already rebasing my next branch to catch up with the backlog, so a new v5 of the driver with all the roll-ups works for me. Also not sure if you caught this comment from earlier: > sidenote: this driver needs to be converted to a dma descriptor pool > (allocated at device_alloc_chan_resources time), or at a minimum use > GFP_NOWAIT in device_prep_dma_memcpy because that routine may be > called from atomic contexts. Thanks, Dan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [00/12] DMAENGINE: PL08X driver updates 2010-08-31 12:12 [00/12] DMAENGINE: PL08X driver updates Linus Walleij 2010-08-31 12:12 ` [PATCH 01/12] DMAENGINE: remove clock code from PL08X driver Linus Walleij @ 2010-09-08 14:26 ` Linus Walleij 2010-09-18 18:52 ` Dan Williams 1 sibling, 1 reply; 20+ messages in thread From: Linus Walleij @ 2010-09-08 14:26 UTC (permalink / raw) To: Dan Williams, linux-arm-kernel, yuanyabin1978; +Cc: linux-kernel 2010/8/31 Linus Walleij <linus.walleij@stericsson.com>: > This updates the PL08X driver in the async_tx next branch with > a number of fixes and features, each one pretty self-describing. > This has been tested with the PrimeCell DMA series on the > RealView PB11MPCore and works flawlessly, multiplexing > several virtual clients, both devices (slaves) and memcpy > channels, across only two (!) available physical channels. Dan can you ACK and queue this for next? I know you're busy, just a reminder... Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [00/12] DMAENGINE: PL08X driver updates 2010-09-08 14:26 ` [00/12] DMAENGINE: PL08X driver updates Linus Walleij @ 2010-09-18 18:52 ` Dan Williams 0 siblings, 0 replies; 20+ messages in thread From: Dan Williams @ 2010-09-18 18:52 UTC (permalink / raw) To: Linus Walleij; +Cc: linux-arm-kernel, yuanyabin1978, linux-kernel On Wed, Sep 8, 2010 at 7:26 AM, Linus Walleij <linus.ml.walleij@gmail.com> wrote: > 2010/8/31 Linus Walleij <linus.walleij@stericsson.com>: > >> This updates the PL08X driver in the async_tx next branch with >> a number of fixes and features, each one pretty self-describing. >> This has been tested with the PrimeCell DMA series on the >> RealView PB11MPCore and works flawlessly, multiplexing >> several virtual clients, both devices (slaves) and memcpy >> channels, across only two (!) available physical channels. > > Dan can you ACK and queue this for next? > > I know you're busy, just a reminder... > I'm back at work this coming week and will get all this queued up. -- Dan ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2010-09-23 15:35 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-31 12:12 [00/12] DMAENGINE: PL08X driver updates Linus Walleij 2010-08-31 12:12 ` [PATCH 01/12] DMAENGINE: remove clock code from PL08X driver Linus Walleij 2010-08-31 12:12 ` [PATCH 02/12] DMAENGINE: safeguard PL08X from missing platform data Linus Walleij 2010-08-31 12:12 ` [PATCH 03/12] DMAENGINE: fetch tasklet struct for PL08X include file Linus Walleij 2010-08-31 12:12 ` [PATCH 04/12] DMAENGINE: use GFP_NOWAIT for PL08X allocations Linus Walleij 2010-08-31 12:12 ` [PATCH 05/12] DMAENGINE: terminate transfers on PL08X mere throughly Linus Walleij 2010-08-31 12:12 ` [PATCH 06/12] DMAENGINE: free txd list if submission fails on PL08X Linus Walleij 2010-08-31 12:12 ` [PATCH 07/12] DMAENGINE: add debugfs file for PL08X Linus Walleij 2010-08-31 12:12 ` [PATCH 08/12] DMAENGINE: be more explicit when freeing PL08X signals Linus Walleij 2010-08-31 12:12 ` [PATCH 09/12] DMAENGINE: remove pointless module safeguard on PL08X Linus Walleij 2010-08-31 12:12 ` [PATCH 10/12] DMAENGINE: move the PL08X to full runtime allocation Linus Walleij 2010-08-31 12:12 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Linus Walleij 2010-08-31 12:12 ` [PATCH 12/12] DMAENGINE: define channel states for the PL08X Linus Walleij 2010-09-23 4:59 ` [PATCH 11/12] DMAENGINE: let PL08X memcpy TXDs wait Dan Williams 2010-09-23 7:39 ` Linus Walleij 2010-09-23 15:29 ` Dan Williams 2010-09-23 7:46 ` Linus Walleij 2010-09-23 15:35 ` Dan Williams 2010-09-08 14:26 ` [00/12] DMAENGINE: PL08X driver updates Linus Walleij 2010-09-18 18:52 ` Dan Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).