This creates a DMAengine driver for the ARM PL080/PL081 PrimeCells
based on the implementation earlier submitted by Peter Pearse.
This is working like a charm for memcpy on the PB11MPCore, but
slave DMA to devices is still not working.

This DMA controller is used in mostly unmodified form in the ARM
RealView and Versatile platforms, in the ST-Ericsson Nomadik, and
in the ST SPEAr platform.

It has been converted to use the header from the Samsung PL080
derivate instead of its own defintions, and can potentially support
several controllers in the same system.

Cc: Peter Pearse <peter.pearse@arm.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/include/asm/hardware/pl080.h |    2 +
 drivers/dma/Kconfig                   |    9 +
 drivers/dma/Makefile                  |    1 +
 drivers/dma/amba-pl08x.c              | 1925 +++++++++++++++++++++++++++++++++
 include/linux/amba/pl08x.h            |  173 +++
 5 files changed, 2110 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/amba-pl08x.c
 create mode 100644 include/linux/amba/pl08x.h

diff --git a/arch/arm/include/asm/hardware/pl080.h b/arch/arm/include/asm/hardware/pl080.h
index f70e1e9..f35b86e 100644
--- a/arch/arm/include/asm/hardware/pl080.h
+++ b/arch/arm/include/asm/hardware/pl080.h
@@ -68,6 +68,8 @@
 #define PL080_CONTROL_TC_IRQ_EN			(1 << 31)
 #define PL080_CONTROL_PROT_MASK			(0x7 << 28)
 #define PL080_CONTROL_PROT_SHIFT		(28)
+#define PL080_CONTROL_PROT_CACHE		(1 << 30)
+#define PL080_CONTROL_PROT_BUFF			(1 << 29)
 #define PL080_CONTROL_PROT_SYS			(1 << 28)
 #define PL080_CONTROL_DST_INCR			(1 << 27)
 #define PL080_CONTROL_SRC_INCR			(1 << 26)
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 8344375..f4655c2 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -36,6 +36,15 @@ comment "DMA ...
[ message continues ]

Linus,

I haven't reviewed it completely. Will do it in some time.


On 6/11/2010 8:57 PM, Linus WALLEIJ wrote:



I agree that there are no request lines from memories but still we can program
them with burst in order to faster the transfer. This burst feature is



above 3 fns are always called in order, i.e. pl08x_enable_phy_chan will
be called after pl08x_set_cregs, so we may not require these checks

can we use pl08x_phy_channel_busy() instead of above code?





Not sure, if we should write above function prototype in just 2 lines,


One more thing linus, why do we need to create this separation between
channels. i.e. few are for memcpy and few for slave_sg. Why don't all

above 2 functions are almost exactly same, can we have single function



better to use dev_get_platdata, also no need of typecasting as

"PL080 or PL081 have just 2"?? You mentioned at the beginning of this

--

[ message continues ]

Hi Viresh, thanks a lot for reviewing this and I'd be *very* happy if
you could give it a spin on
the SPEAr as well!



Actually in the example platform data I set this to the maxburst size
256 Bytes, however if I read the manual correctly this is simply ignored
when you do mem2mem transfers.

It will simply AHB master the bus until the transaction is finished. That
is why the manual states:

"You must program memory-to-memory transfers with a low channel
priority, otherwise:
•    other DMA channels cannot access the bus until the
memory-to-memory transfer
     has finished



The previous check if the channel is active before proceeding, this check also
checks the enable bit, this behaviour comes straight from the manual and is



Probably but the compiler will do that anyway if there some





This is done in all in-tree drivers, the reason is (I think) that for example
the dmatest.c test client will look for:

	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
		cnt = dmatest_add_threads(dtc, DMA_MEMCPY);
		thread_count += cnt > 0 ? cnt : 0;

So if you want to partition some channels for device I/O (typically some
which are hard-coded to some devices) and others for memcpy() you create
a slave instance for the former and a memcpy() instance for the latter.

In this case we multiplex the memcpy and slave transfers on the few
physical channels we have, but I haven't finally decided how to handle this:
perhaps we should always set on physical channel aside for memcpy
so this won't ever fail, and then this special memcpy device entry will help.






Wrong words again, this is one of the most confusing things about PL080/PL081,
it has 2 or 8 *channels* but always 16 *signals*.

Almost all other interrupt controllers have a 1-to-1 correspondence between the
number of incoming signals and the number of available slave channels.
Not the PL08x... it has less channels than signals.

I will underscore it again... if it confuses both me and you it will ...
[ message continues ]

I would be happy too linus, will do it in few weeks, right now we are running

Yes i have seen them. One more thing i have found related to mem to mem
transfers is:
"You must set this value to the burst size of the destination peripheral,

I was talking about the similar check present in pl08x_set_cregs, which will
be called before this routine. pl08x_set_cregs has already checked if channel

Hmmm. I am not sure, but i think we can't hard code a channel for some device.
All channels should be available with both capabilities. If still there are
some conditions (that you might know), where we need to hard code channels
for devices, then this should come from plat data in some way.



I have few more doubts that i wanted to ask. Are following supported in your
driver, we need them in SPEAr:
 - Configure burst size of source or destination.
 - Configure DMA Master for src or dest.
 - Transfer from Peripheral to Peripheral.
 - Configure Width of src or dest peripheral.
 - Configure Flow controller of transfer.
 - Some callback for fixing Request line multiplexing just before
	initiating transfer.
 - Multiple sg elements in slave_sg transfer. I think it is not supported.
 - Control for autoincrement of addresses, both in case of memory and
	peripherals.


viresh.
--

[ message continues ]

Yeah I know that feeling ... anyway I will probably publish a few

Currently I don't hardcode anything, the physical channels
(on the PL081 only two!) will be multiplexed on a first-come
First-served basis. This is a bit problematic since if I start
a DMAengine memcpy test there is a real battle about the channels...
The memcpy test assumes it will always get a channel, see.

I could queue the transfers waiting for a physical channel to

The PrimeCell extension supports this, do you need that in things

Right now I have an algorithm that will (on the PL080, the PL081
has only one master) try to select AHB1 for the memory and AHB2
for the device by checking if one address is fixed. If both or
none addresses are fixed it will simply select AHB1 for source
and AHB2 for destination.




Currently only done dynamically with DMA as the master for
Mem2mem, mem2per and per2mem. Mastering from the peripherals
is not supported. Do you have advanced features like that?


This is part of this driver. RealView & versatile have exactly


Right now the engine autoincrements the memory pointers if memory
is source/destination and both on mem2mem.

If you actually have peripherals that need increasing pointers it can
Probably be added.

Yours,
Linus Walleij
--

[ message continues ]

In the same way, how other peripheral related data is passed to DMA driver,
(like request lines), we can also pass configuration and control information.
This will provide us with all features requested by me, as most of the
control will be from user only. In SPEAr6xx, Memory is accessible from Master1
only but in SPEAr3xx only from Master 2, similar is the pattern with few 

We have JPEG controller, which acts as a flow controller for JPEG to

But platform data will be passed one time only and we will not be able to
do it while transferring data at run time.
--

[ message continues ]

Oh I had no clue that you could set up your masters like that!
Anyway, I'll attempt to hack in some platform config for how
the AHB masters are assigned, but you'll likely have to patch it


Usually there is a very fixed use for each virtual DMA channel (which
have a platform config each), i.e. usually there is only one or two
flow controls per virtual channel. So in this case I guess that
Synopsys JPEG has a virtual channel that always is JPEG->mem with
JPEG as master, so it can actually be in fix platform data?

Anyway, we can probably extended either the way we did for PrimeCells
or in some generic way by adding config commands to the DMAengine,
so I see no road blocker.

Yours,
Linus Walleij
--

[ message continues ]

Looks good, but please give me some time to test on the board(SMDK6410).
(snip)

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

--

[ message continues ]

Samsung doesn't use the DMA API, so this driver is unlikely to work.
--

[ message continues ]

It doesn't indeed, but it could be adapted to be a wrapper around the DMA engine API.
Or even better, the drivers could be adapted to use that API.
--

[ message continues ]

On Tue, Jun 15, 2010 at 8:17 PM, Maurus Cuelenaere
I don't particularly like the idea of making Samsung's drivers use the DMA API.
IMHO the S3C dma api is better atm.
--

[ message continues ]

At the moment it is better, true. But I do think it'd be better to use a
generic API than an arch-specific one.
I'm not familiar enough with both of them to know what functionality is
currently missing but I do think that once the DMA engine API is up to
speed, it should replace the s3c one.

-- 
Maurus Cuelenaere

--

[ message continues ]

Yeah that was never the intention, the Samsung derivate has one extra
Register and other funny stuff.

I was more hoping on breaking out a subset in the same elegant way
with a generic part and a separate DMAengine as was done with the
PL330... But I need to get it fully working first.

Atleast I used the same include file.

But I have functional UART DMA on the RealView PB11MPCore now!

Yours,
Linus Walleij
[ message continues ]

Having just looked at this while trying to undo the DMA API abuses
in the PL011 UART driver, I'm getting rather frustrated with this
code.

What's wrong with the PL08x DMA engine driver?  Well, in the PL011
UART driver, you do this:

+static void pl011_dma_tx_callback(void *data)
+{
...
+       /* Refill the TX if the buffer is not empty */
+       if (!uart_circ_empty(xmit)) {
+               ret = pl011_dma_tx_refill(uap);
...
+static int pl011_dma_tx_refill(struct uart_amba_port *uap)
+{
...
+       /* Prepare the scatterlist */
+       desc = chan->device->device_prep_slave_sg(chan,
+                                                 &dmatx->scatter,
+                                                 1,
+                                                 DMA_TO_DEVICE,
+                                                 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
...
+       /* Some data to go along to the callback */
+       desc->callback = pl011_dma_tx_callback;
+       desc->callback_param = uap;

Note that this calls the channel device_prep_slave_sg() from the
callback.  This seems reasonable.

Right, now let's look at this driver (from the latest kernel):

static void pl08x_tasklet(unsigned long data)
{
...
        spin_lock(&plchan->lock);
...
                dma_async_tx_callback callback =
                        plchan->at->tx.callback;
                void *callback_param =
                        plchan->at->tx.callback_param;
...
                /*
                 * Callback to signal completion
                 */
                if (callback)
                        callback(callback_param);

Note that the callback is called with the channel lock held.

struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
                struct dma_chan *chan, struct scatterlist *sgl,
                unsigned int sg_len, enum dma_data_direction direction,
                unsigned long flags)
{
...
        ret = pl08x_prep_channel_resources(plchan, txd);
        if ...
[ message continues ]

Here's another couple of problems:

PL011 driver:

+	dmatx->cookie = desc->tx_submit(desc);
+	if (dma_submit_error(dmatx->cookie)) {
+		/* "Complete" DMA (errorpath) */
+		complete(&dmatx->complete);
+		chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
+		return dmatx->cookie;
+	}

dmatx->cookie is of type dma_cookie_t, which is a s32.

MMCI driver:

+	dma_cookie_t cookie;
...
+	cookie = desc->tx_submit(desc);
+
+	/* Here overloaded DMA controllers may fail */
+	if (dma_submit_error(cookie))
+		goto unmap_exit;

#define dma_submit_error(cookie) ((cookie) < 0 ? 1 : 0)

So, DMA errors are negative values returned from the tx_submit function.

PL08x tx_submit method:

static dma_cookie_t pl08x_tx_submit(struct dma_async_tx_descriptor *tx)
{
        struct pl08x_dma_chan *plchan = to_pl08x_chan(tx->chan);

        atomic_inc(&plchan->last_issued);
        tx->cookie = atomic_read(&plchan->last_issued);
        /* This unlock follows the lock in the prep() function */
        spin_unlock_irqrestore(&plchan->lock, plchan->lockflags);

        return tx->cookie;
}

So, after 2^31 DMA transactions, the DMA engine layer continues happily
along with the queued DMA operation, but the driver gets a negative
cookie returned, and so bails out.

I'm also left wondering about that atomic_t.  It's incremented and
read in a locked region, which will in itself prevent two concurrent
increments.  The only other places its used is from dma_tx_status()
where it is only ever read, and on the initialization path.  That to
me makes no sense.

I'll see if I can get to looking at the PL08x stuff once I've finished
sorting out the PL011 UART driver DMA support into a state where I'm
happy that it's doing things correctly.
--

[ message continues ]

Right, just tried this on the Versatile PB/926, which has a PL080.
The result is DMA errors.  This turns out to be the hard-coding of
which AHB bus is used.

You can't hard-code this information into the driver - it's part of
the bus matrix configuration.  On Versatile PB/926, the two AHB
buses have different memory maps - see DUI0224 page 3-13:

* DMA0 (which is DMA AHB M1) has access to the APB peripherals
  but not the system memory.
* DMA1 (which is DMA AHB M2) has access to the system memory but
  none of the APB peripherals.

Throwing in #ifdef's to sort this out resolves the problem on the PB/926.

Looking at the driver code too, I'm having a hard time understanding it,
probably because of the use of "master" and "slave" - I don't think this
has anything to do with which AHB master is used.  The comments against
pl08x_choose_master_bus() seem to imply that it controls which AHB master
is used, but it has no apparant effect on that.  I don't think this
function does anything like that anymore.
--

[ message continues ]

Having found one of my ARM platforms which actually supports DMA (such
a rare thing - neither my Realview EB nor Versatile Express supports it),
I can finally see about run-time checking some of this.

BUG: spinlock lockup on CPU#0, sh/417, c1870a08
Backtrace:
[<c0038880>] (dump_backtrace+0x0/0x10c) from [<c02c20e0>] (dump_stack+0x18/0x1c) r7:c104e000 r6:c1870a08 r5:00000000 r4:00000000
[<c02c20c8>] (dump_stack+0x0/0x1c) from [<c017b520>] (do_raw_spin_lock+0x118/0x154)
[<c017b408>] (do_raw_spin_lock+0x0/0x154) from [<c02c4b98>] (_raw_spin_lock_irqsave+0x54/0x60)
[<c02c4b44>] (_raw_spin_lock_irqsave+0x0/0x60) from [<c01f5828>] (pl08x_prep_channel_resources+0x718/0x8b4)
 r7:c1870a08 r6:00000001 r5:c19c3560 r4:ffd04010
[<c01f5110>] (pl08x_prep_channel_resources+0x0/0x8b4) from [<c01f5bb4>] (pl08x_prep_slave_sg+0x120/0x19c)
[<c01f5a94>] (pl08x_prep_slave_sg+0x0/0x19c) from [<c01be7a0>] (pl011_dma_tx_refill+0x164/0x224)
 r8:00000020 r7:c03978c0 r6:c1850000 r5:c1847e00 r4:c1847f40
[<c01be63c>] (pl011_dma_tx_refill+0x0/0x224) from [<c01bf1c8>] (pl011_dma_tx_callback+0x7c/0xc4)
[<c01bf14c>] (pl011_dma_tx_callback+0x0/0xc4) from [<c01f4d34>] (pl08x_tasklet+0x60/0x368)
 r5:c18709a0 r4:00000000
[<c01f4cd4>] (pl08x_tasklet+0x0/0x368) from [<c004d978>] (tasklet_action+0xa0/0x100)
[<c004d8d8>] (tasklet_action+0x0/0x100) from [<c004e01c>] (__do_softirq+0xa0/0x170)
 r8:00000006 r7:00000001 r6:00000018 r5:c104e000 r4:00000100
[<c004df7c>] (__do_softirq+0x0/0x170) from [<c004e140>] (irq_exit+0x54/0x9c)
[<c004e0ec>] (irq_exit+0x0/0x9c) from [<c0029080>] (asm_do_IRQ+0x80/0xa0)
[<c0029000>] (asm_do_IRQ+0x0/0xa0) from [<c00345b8>] (__irq_svc+0x38/0xa0)

As described above in my code analysis, pl08x_tasklet takes the spinlock,
calls pl011_dma_tx_callback and eventually back to pl08x_prep_slave_sg
and pl08x_prep_channel_resources which then try to take the spinlock
again, leading to deadlock.
--

[ message continues ]

On Wed, Dec 22, 2010 at 4:29 AM, Russell King - ARM Linux

This is listed in the dmaengine documentation [1], but I obviously
missed this before merging.  This also would have been caught by
lockdep as required by SubmitChecklist.  As far as corrective action
before 2.6.37-final.  It looks like this driver needs a full scrub
which seems unreasonable to complete and test over the holidays before
.37 lands.  Linus we either need to mark this "depends on BROKEN" or
revert it.

Support for the DMA_COMPL flags are necessary if the DMA_MEMCPY
capability is advertised, yes this driver got this wrong.  I'll update
the documentation to make this requirement clear, and audit the other
drivers.  With slave-only drivers the only usage model is one where
the client driver owns dma-mapping.  In the non-slave (opportunistic
memcpy offload) case the client is unaware of the engine so the driver
owns unmapping.  The minimal fix is to disable memcpy offload.

--
Dan

[1]
3.6 Constraints:
1/ Calls to async_<operation> are not permitted in IRQ context.  Other
   contexts are permitted provided constraint #2 is not violated.
2/ Completion callback routines cannot submit new operations.  This
   results in recursion in the synchronous case and spin_locks being
   acquired twice in the asynchronous case.
--

[ message continues ]

(2) seems to be more than a little annoying - it seems that DMA engine
drivers use a tasklet for running their DMA cleanup, which calls drivers
callbacks, and we're going to have to have a whole pile of taskets
in drivers just to be triggered from the completion callback.  I can
see this adding an additional layer of complexity and a nice fine set
of shiney new races to deal with.
--

[ message continues ]

On Wed, Dec 22, 2010 at 3:54 PM, Russell King - ARM Linux

I should clarify, this is the async_memcpy() api requirement which is
not used outside of md/raid5.  DMA drivers can and do allow new
submissions from callbacks, and the ones that do so properly move the
callback outside of the driver lock.  The doc needs updating to
reflect present reality, but it at least should have prompted the same
reaction you had when reading it and triggered a question about how to
support that usage model.

--
Dan
--

[ message continues ]

BTW, this is misleading.  Have the functions been renamed dma_async_xxx(),
eg dma_async_memcpy_buf_to_buf etc, or are you referring just to:

	async_dmaengine_get
	async_dmaengine_put
	async_dma_find_channel
	async_dma_find_channel
	async_tx_ack
	async_tx_clear_ack
	async_tx_test_ack

Beware of just renaming it to dma_async_<operation> as there's other
functions in that namespace which may not be appropriate.

Eg, is it really illegal to issue call dma_async_issue_pending() from
IRQ context?  That'd make it exceedingly difficult to use the DMA
engine with the slave API in a lot of device drivers.
--

[ message continues ]

On Wed, Dec 22, 2010 at 4:10 PM, Russell King - ARM Linux

This is generic offload (async_{memcpy|xor|etc...}) versus the slave
usage confusion .  In the async_<operation> case the client (md/raid5)
has no idea if a dmaengine is offloading the operation behind the
scenes and should not make any assumptions about submission context
beyond what is allowed in the document.  In the slave case the client
driver knows that it is talking to a specific dma driver.  The
contract between the client driver and dma driver is undocumented.
The slave usage model really is a "I want to use dmaengine to find my
dma device driver / manage channels, and I want a common prep / submit
mechanism, but otherwise stay out of my way".  Drivers that do not
want to meet the constraints expected by the opportunistic offload
clients should do what ste_dma40 does and add "depends on !(NET_DMA ||
ASYNC_TX_DMA)"

--
Dan
--

[ message continues ]

Sorry I was looking at a local change it does not do this today, but I
recommended it to workaround the broken >64k transfer support that was
reported recently.

--
Dan
--

[ message continues ]

Dan,

Is there any documentation on the DMA engine APIs than what's in
crypto/async-tx-api.txt?

Reason for asking is that there's no way at the moment to tell what the
expectations are from a lot of the DMA engine support code - and that is
_very_ bad news if you want DMA engine drivers to behave the same.

I can already see that drivers on both sides of the DMA engine API have
different expectations between their respective implementations, and this
is just adding to confusion.

For instance, the sequence in a driver:

	desc = dmadev->device_prep_slave_sg(chan, ...);
	if (!desc)
		/* what DMA engine cleanup is expected here? */

	cookie = dmaengine_submit(desc);
	if (dma_submit_error(cookie))
		/* what DMA engine cleanup of desc is expected here? */

Note: I don't trust what's written in 3.3 of async-tx-api.txt, because
that seems to be talking about the the async_* APIs rather than the
DMA engine API. (see below.)

1. Is it valid to call dmaengine_terminate_all(chan) in those paths?

2. What is the expectations wrt the callback of a previously submitted
   job at the point that dmaengine_terminate_all() returns?

3. What if the callback is running on a different CPU, waiting for a
   spinlock you're holding at the time you call dmaengine_terminate_all()
   within that very same spinlock?

4. What if dmaengine_terminate_all() is running, but parallel with it
   the tasklet runs on a different CPU, and queues another DMA job?

These can all be solved by requiring that the termination implementations
call tasklet_disable(), then clean up the DMA state, followed by
tasklet_enable().  tasklet_disable() will prevent the tasklet being
scheduled, and wait for the tasklet to finish running before proceeding.
This means that (2) becomes "the tasklet will not be running", (3)
becomes illegal (due to deadlock), and (4) becomes predictable as we
know that after tasklet_disable() we have consistent DMA engine state
and we can terminate everything that's been ...
[ message continues ]

On Fri, Dec 31, 2010 at 1:50 PM, Russell King - ARM Linux

No, the dma slave usage model is undocumented, and the generic offload
case needs updating.  I will write up the results of this thread in

For the generic offload usage case dma drivers need to present
consistent behaviour because they are reused generically.

The slave usages are not generic.  They grew up around wanting to
reuse dmaengine's channel allocation boilerplate, while maintaining
architecture-specific (dma driver / slave driver pairing) behaviour.
Certainly the ->device_prep* methods can be made to present a
consistent/documented interface.  The ->device_control() method, on
the other hand, defines a common interface to pass control messages,
but the semantics are implementation specific (slave drivers take
liberties with the knowledge that they will only ever talk to a given

None, if prep fails then no descriptor was allocated and no cleanup

dma_submit_error() is something I should have removed after commit
a0587bcf "ioat1: move descriptor allocation from submit to prep" all
errors should be notified by prep failing to return a descriptor
handle.  Negative dma_cookie_t values are only returned by the

This assumes that all submission and completion occurs in tasklet
context?  I think something is wrong if the terminate_all
implementation is not taking the channel spinlock or otherwise
guaranteeing that it is not racing against new submissions and the

A single thread must not call any other dmaengine methods between prep
and submit.  Some drivers lock in prep and unlock in submit as they
need to ensure in-order submission.  The only reason they are separate
calls is to preclude needing to specify callback information to every
prep call as some usages will never need it.

Now for the case of ->prep() on cpu0 with an intervening call to
terminate_all on cpu1 before the submit.  I would say that the
terminate_all operation should ignore the yet-to-be-submitted

Proper handling of that flag is only ...
[ message continues ]

The device_prep/submit/terminate/pause/resume interface is really what
I was referring to - the behaviour of these needs to be really well

Ok, that means error checking on the dmaengine_submit() result in slave

I was initially thinking more of the case where we have:

	tasklet->callback->attempted submission of next tx buffer,
		submission failure->terminate_all->tasklet_disable

As tasklet_disable() will wait for the tasklet to finish running, this
results in deadlock.  This is exactly what could happen with the PL011
UART driver's usage of the DMA engine API.  However, as a result of your
comments, I've removed the terminate_all calls in those failure paths as
they're entirely unnecessary.  So that problem is solved.

However, that is not the only place where this can happen:

CPU0		CPU1
		takes slave driver spinlock
tasklet
callback
spins on slave driver spinlock
		terminate_all
		tasklet_disable

That also leads to deadlock - and again is something still to be solved
between the PL011 UART driver and PL08x DMA driver.

This scenario also applies if you do similar things in the DMA engine
driver.  Most DMA engine drivers take a spinlock within their tasklet.

CPU0		CPU1
		terminate_all
tasklet
		takes DMA engine driver spinlock
spins on DMA engine driver spinlock
		tasklet_disable

So... beware of DMA engine drivers which use tasklet_disable() in their


Ok, so that's something else which needs fixing in PL08x.
--

[ message continues ]

Such is life, I am still very happy that you have been taking some
time to dive into this. I will try to work through the points raised
as far as I can...

As for the in-tree PL08x driver I'd say it's doing pretty well for
memcpy() so we could add platform data for that on supported
platforms, then for device transfers we need more elaborative
work.

I will also try to get the Nomadik 8815 up to get some reference
HW for this combo that actually works, including device transfers on
MMCI etc.

Yours,
Linus Walleij
--

[ message continues ]

It has the issue that it's not unmapping the buffers after the memcpy()
operation has completed, so on ARMv6+ we have the possibility for
speculative prefetches to corrupt the destination buffer.

Neither are a number of the other DMA engine drivers.  This is why I'd
like to see some common infrastructure in the DMA engine core for saying
"this tx descriptor is now complete" so that DMA engine driver authors
don't have to even think about whether they should be unmapping buffers.

I'd also like to see DMA_COMPL_SKIP_*_UNMAP always set by prep_slave_sg()
in tx->flags so we don't have to end up with "is this a slave operation"
tests in the completion handler.
--

[ message continues ]

My point is that async_<operation> refers to something that doesn't
exist.  What we do have:

dma_cookie_t dma_async_memcpy_buf_to_buf(struct dma_chan *chan,
        void *dest, void *src, size_t len);
dma_cookie_t dma_async_memcpy_buf_to_pg(struct dma_chan *chan,
        struct page *page, unsigned int offset, void *kdata, size_t len);
dma_cookie_t dma_async_memcpy_pg_to_pg(struct dma_chan *chan,
        struct page *dest_pg, unsigned int dest_off, struct page *src_pg,
        unsigned int src_off, size_t len);

Nothing in dmaengine.h matches 'async_xor', so that suggests it's
incomplete.  There's another problem with referring to them as
dma_async_<operation> because we also have:

static inline void dma_async_issue_pending(struct dma_chan *chan)
#define dma_async_memcpy_issue_pending(chan) dma_async_issue_pending(chan)
static inline enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
        dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
#define dma_async_memcpy_complete(chan, cookie, last, used)\
static inline enum dma_status dma_async_is_complete(dma_cookie_t cookie,
                        dma_cookie_t last_complete, dma_cookie_t last_used)
int dma_async_device_register(struct dma_device *device);
void dma_async_device_unregister(struct dma_device *device);

So just referring to dma_async_<operation> will also be confusing.

There's also a bunch of other async_* named functions which makes it
look like these are what's being referred to (see the list above).
So, the documentation is confusing.

Maybe it would be better to refer to struct dma_device's device_prep_dma_*
methods and anything which calls them, with the exception of
device_prep_dma_cyclic?  (Shouldn't that be device_prep_slave_cyclic?)
--

[ message continues ]

Yeah, my bad. I'll get better at this... :-(
(I blame it partially on inaccessible hardware, sob sob. I do like to

Isn't it really as simple as to release the spinlock during callbacks?
That lock is only intended to protect the plchan variables, not to block
anyone from queueing new stuff during the callback (as happens now).

It can release that lock, make a callback where a new descriptor
gets queued, and then take it again and start looking at the queue,
at which point it discovers the new desc and process it.

So something like this:


From: Linus Walleij <linus.walleij@stericsson.com>
Date: Thu, 23 Dec 2010 09:06:14 +0100
Subject: [PATCH] dma: release pl08x channel lock during callback

The spinlock is not really safeguarding any resources during the
callback, so let's release it before that and take it back
afterwards so as to avoid deadlocks.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/dma/amba-pl08x.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index b605cc9..7879a22 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1651,8 +1651,11 @@ static void pl08x_tasklet(unsigned long data)
 		/*
 		 * Callback to signal completion
 		 */
-		if (callback)
-			callback(callback_param);
+		if (callback) {
+                        spin_unlock(&plchan->lock);
+                        callback(callback_param);
+                        spin_lock(&plchan->lock);
+                }

 		/*
 		 * Device callbacks should NOT clear
-- 
1.7.2.3

Yours,
Linus Walleij
--

[ message continues ]

On Thu, Dec 23, 2010 at 5:17 PM, Linus Walleij
How about adding completed requests to a list and go on to do important
channel management stuff, and do callbacks at the end after dropping the lock.
As in pl330_tasklet of drivers/dma/pl330.c
--

[ message continues ]

Is it actually safe to do this?  The answer seems to be no - if we happen
to terminate all transfers (as your PL011 uart code does) when we fail to
setup a new DMA transaction, then bad stuff happens due to this:

                /*
                 * Device callbacks should NOT clear
                 * the current transaction on the channel
                 * Linus: sometimes they should?
                 */
                if (!plchan->at)
                        BUG();

We really need a saner approach here - maybe the list approach described

Plus, of course, that tasklets run with IRQs enabled.  This means we're
taking this spinlock in an interruptible context.  If we have some other
path which also takes this lock from an IRQ context, then we're asking
for deadlock.  See my previous mails on this subject.

I'm currently splitting my dirty patch, and attacking this driver to
clean up some of it into a more reasonable shape, so this is one area
which I'm going to be sorting out.

Patches later.
--

[ message continues ]

Thanks Russell, much appreciated. I'll be quick to review and ACK
any patches hopefully.

Yours,
Linus Walleij
--

[ message continues ]

As a side note, the DMA mapping for slaves should be done using the
DMA struct device, not the struct device of the peripheral making use
of the DMA engine.

Why?  The slave device has no knowledge of how the DMA engine is
connected into the system, or the DMA parameters associated with the
device performing the DMA, such as the DMA mask and boundaries.  (If
there are several generic DMA agents in the system, it can't know
which is the correct one to use until a channel has been allocated.)
The only struct device which has this information is the one for the
DMA engine itself.

Therefore, the struct device which is passed into the DMA mapping APIs
to prepare memory for DMA must always be the DMA engine struct device
(chan->device->dev) and never the slave struct device.

This is no different from USB - consider the slave devices to be USB
peripherals, and the DMA engine device to be the USB host controller.
The USB host controller performs all the DMA, and DMA mappings are
setup and torn down against the DMA host controller device structure.
--

[ message continues ]

FYI I patched the amba-pl022.c for this (and your other immediate
comments) and it's queued for the next merge window.

Yours,
Linus Walleij
--

[ message continues ]

Another point - the example code in async-tx-api.txt section 3.7 is
a very bad example.  It doesn't show how the result of one operation
is passed to the next operation, as the source and destination for
each operation is passed in separately.  It actually won't even
compile because of this declaration:

        addr_conv_t addr_conv[xor_src_cnt];
        addr_conv_t addr_conv[NDISKS];

Neither of these are used in the example.

On the subject of chained operations, I really don't see how this can
hope to work with the DMA API.  Using the example provided there:

	async_xor(dst, src, ...)
	async_memcpy
	async_xor(dst, src, ...)
	async_tx_issue_pending_all

Let's assume that the operations start running when we call
async_tx_issue_pending_all(), and the two XOR operations reuse the same
buffers.

The first async_xor() dma_map_page()'s the source and destination buffers.
At this point, the ownership of these buffers passes to the DMA device.

When we get to the second async_xor(), as we haven't started to run any
of these operations, the source and destination buffers are still mapped.
However, we ignore that and call dma_map_page() on them again - this is
illegal because the CPU does not own these buffers.

Moreover, when the first XOR operation completes, it will unmap the
buffers (which returns ownership of the buffer to the CPU), but
continues with the second XOR operation on a now unmapped buffer.

If there is an IOMMU between the DMA engine peripheral and memory, then
this is going to be really vile.  As it is, I don't think this is going
to be reliable on ARM as we're destroying the ownership rules for DMA
buffers which we rely heavily upon to prevent effects from speculative
prefetching.

Last point I've noticed reading through this example and the associated
code is that the async_xor() maps the destination buffer using
DMA_BIDIRECTIONAL.  The corresponding unmap (in the DMA engine driver)
_must_ also be done with DMA_BIDIRECTIONAL (I don't see any ...
[ message continues ]

Another issue to think about in terms of API guarantees...

Pausing an on-going transfer (which from what I read is a recent addition
to the API) can be problematical - having tripped over one such problem
on my Realview EB (PL081 rev1) platform.

With the PL08x, when you program a channel up for a M->P transfer and
enable it, the DMAC will immediately fetch data from memory and mark
itself busy before the first DMA request from the peripheral.

When the first DMA request occurs from the peripheral, the DMAC starts
feeding this data to the peripheral, and fetches new data into its FIFO
as required.

The problem comes when the peripheral stops requesting data from the
DMAC, and then you want to pause it.  Current code sets the HALT bit,
and then spins indefinitely for the BUSY bit to clear - which it will
only ever do if the peripheral drains the FIFO.  As a result of botched
DMA signal selection code for the Realview EB, the DMA signals never
went active, and on terminate_all() the CPU locked up solid (no
interrupts) spinning on the DMAC to clear the busy bit.

It may be worth specifying that a pause followed by a resume is
expected to never lose data - and that drivers must check the result
of a request to pause the channel.  If DMA engine drivers are unable
to pause the channel within a reasonable amount of time, they should
return -ETIMEOUT, so they know that there may still be data that
could still be transferred to the peripheral.

One important note about this condition is that with the DMA channel
marked BUSY+HALT and the channel being configured for M->P, the
peripheral can still transfer data from the DMAC to itself, and this
causes the transfer size value in the CNTL register to decrement (since
reading the CNTL register returns the number of transfers _to_ the
destination, not the number of transfers from the source.)

I've changed the comment against pl08x_pause_phy_chan() to this:

 * Pause the channel by setting the HALT bit.
 *
 * For M->P transfers, ...
[ message continues ]

On Tue, Jan 4, 2011 at 12:19 AM, Russell King - ARM Linux

Not sure if every DMAC could resume cleanly from exact pause point.
Perhaps the upper layer of client driver should take care of that, just
like ALSA which emulates pause/resume if the 'optional' capability is
not advertised by the audio dma driver.
--

[ message continues ]

Added by me to handle the PrimeCells actually, but also needed

Argh, how typical.

BTW it's great that you have the EB up, I think it's very close or


So the -ETIMEOUT needs to have the semantic meaning
"data is in flight". Doesn't -EBUSY fit better to describe that,
though it will be caused by a spin-loop timeout?

(OK maybe nitpicky, doesn't really matter as long as we specify
*something*, but see below for the better semantic meaning of


Hmhm. One part of me wants the DMAC to clear the state of that
channel completely if this timeout happens and you return
-ETIMEOUT and not allow resuming, but if you return -EBUSY
as per above, the pause has definately failed and there is
nothing to resume, we're still in flight and the only way to
really stop the transfer from that point is to TERMINATE_ALL.

Yours,
Linus Walleij
--

[ message continues ]