Message-ID: <20110103151944.GA32508@n2100.arm.linux.org.uk>
Date:	Mon, 3 Jan 2011 15:19:44 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Dan Williams <dan.j.williams@...el.com>,
	Linus Walleij <linus.walleij@...ricsson.com>
Cc:	Viresh Kumar <viresh.kumar@...com>,
	Kukjin Kim <kgene.kim@...sung.com>, yuanyabin1978@...a.com,
	linux-kernel@...r.kernel.org, Ben Dooks <ben-linux@...ff.org>,
	Peter Pearse <peter.pearse@....com>,
	linux-arm-kernel@...ts.infradead.org,
	Alessandro Rubini <rubini@...pv.it>
Subject: Re: [PATCH 06/13] DMAENGINE: driver for the ARM PL080/PL081
	PrimeCells

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, pause the DMAC first and then stop the peripheral -
 * the FIFO can only drain if the peripheral is still requesting data.
 * (note: this can still timeout if the DMAC FIFO never drains of data.)
 *
 * For P->M transfers, disable the peripheral first to stop it filling
 * the DMAC FIFO, and then pause the DMAC.

I've not decided whether it should be possible to resume an ETIMEOUT'd
pause request (in theory with pl08x, that's just a matter of clearing
the HALT bit) or whether an ETIMEOUT'd pause request should restore
the previous HALT bit setting (possibly re-enabling transfers on the
channel.)  Allowing it to be re-enabled in some way would be the safest
thing in terms of data integrity for the reason mentioned in the
"important note" above.

Dan, Linus, any thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists