diff mbox

[10/18] spi: s3c64xx: Do not require legacy DMA API in case of S3C64XX

Message ID 1376243970-6489-11-git-send-email-tomasz.figa@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa Aug. 11, 2013, 5:59 p.m. UTC
With support for amba-pl08x driver, on S3C64xx the generic DMA engine
API can be used instead of the private s3c-dma interface.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Brown Aug. 12, 2013, 11:36 p.m. UTC | #1
On Sun, Aug 11, 2013 at 07:59:22PM +0200, Tomasz Figa wrote:
> With support for amba-pl08x driver, on S3C64xx the generic DMA engine
> API can be used instead of the private s3c-dma interface.

I'm still getting issues for SPI with this:

[   50.952845] s3c64xx-spi s3c6410-spi.0: Failed to get RX DMA channel
[   50.953520] spi_master spi0: failed to prepare transfer hardware

when I test (with -next with this series applied on top), this is from
the first transfer that tries to DMA - there's actually no RX data.  No
errors are reported by the DMA controller itself.  I'm also getting
crashes setting up the audio DMA while derferencecing the channel.

This is without DT - are there any hookups missing because of that?
Tomasz Figa Aug. 12, 2013, 11:49 p.m. UTC | #2
On Tuesday 13 of August 2013 00:36:41 Mark Brown wrote:
> On Sun, Aug 11, 2013 at 07:59:22PM +0200, Tomasz Figa wrote:
> > With support for amba-pl08x driver, on S3C64xx the generic DMA engine
> > API can be used instead of the private s3c-dma interface.
> 
> I'm still getting issues for SPI with this:
> 
> [   50.952845] s3c64xx-spi s3c6410-spi.0: Failed to get RX DMA channel
> [   50.953520] spi_master spi0: failed to prepare transfer hardware
> 
> when I test (with -next with this series applied on top), this is from
> the first transfer that tries to DMA - there's actually no RX data.  No
> errors are reported by the DMA controller itself.  I'm also getting
> crashes setting up the audio DMA while derferencecing the channel.
> 
> This is without DT - are there any hookups missing because of that?

Hmm, strange.

I've been testing this without any significant patches, like PWM, CCF or 
DT support, just everything from the branch I pointed to and some board-
specific patches to help in debugging.

Anyway, this is the exact branch I've been using:

git://github.com/tom3q/linux.git v3.12-pl080-devel

Best regards,
Tomasz
Mark Brown Aug. 13, 2013, 12:02 p.m. UTC | #3
On Tue, Aug 13, 2013 at 01:49:53AM +0200, Tomasz Figa wrote:

> I've been testing this without any significant patches, like PWM, CCF or 
> DT support, just everything from the branch I pointed to and some board-
> specific patches to help in debugging.

I retested with the SPI patches you sent separately yesterday which
didn't make a difference for the SPI issue.

> Anyway, this is the exact branch I've been using:

> git://github.com/tom3q/linux.git v3.12-pl080-devel

I can't test this directly as it has the DAPM bugs I fixed yesterday
in it (those patches will hit -next tomorrow), however it does look like
there's nothing else relevant in there...
Mark Brown Aug. 13, 2013, 6:55 p.m. UTC | #4
On Tue, Aug 13, 2013 at 01:49:53AM +0200, Tomasz Figa wrote:

> I've been testing this without any significant patches, like PWM, CCF or 
> DT support, just everything from the branch I pointed to and some board-
> specific patches to help in debugging.

Done some more digging.  It's not failing the first time it requests the
channel, it's failing sometime later on (after we've been running for a
while but when we've had a substantial idle period).  However it is the
first time we're actually intending to use the channel, previously to
this the transfers would always be too small to trigger DMA.  I'm not
sure that's relevant though as we're not getting far enough to actually
request the channel.

I'm still debugging what's going on here - the basic refcounting all
looks OK in the SPI driver, I can see it requesting and releasing with
the refcounts all going back to zero in the DMA driver as expected but
when we come back to the device later on dmaengine is deciding the
device is unavailable quite early on in the process.
Mark Brown Aug. 13, 2013, 8:18 p.m. UTC | #5
On Tue, Aug 13, 2013 at 07:55:47PM +0100, Mark Brown wrote:

> I'm still debugging what's going on here - the basic refcounting all
> looks OK in the SPI driver, I can see it requesting and releasing with
> the refcounts all going back to zero in the DMA driver as expected but
> when we come back to the device later on dmaengine is deciding the
> device is unavailable quite early on in the process.

The failure is happening because this check is failing:

	/* devices with multiple channels need special handling as we need to
	 * ensure that all channels are either private or public.
	 */
	if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask))
		list_for_each_entry(chan, &dev->channels, device_node) {
			/* some channels are already publicly allocated */
			if (chan->client_count) {

which is happening because dma1chan0 (which is on the same DMA
controller as the SPI controller) and in fact every other DMA channel
had references grabbed by the network stack dmaengine helpers which I'd
enabled in config.  The fact that they do that is unhelpful, it renders
the API mostly useless, but is nothing to do with this series.

Having tweaked the config everything appears to work so:

Tested-by: Mark Brown <broonie@linaro.org>

though the whole thing with the filter function is as I say a bit fun
from a code review point of view.
Tomasz Figa Aug. 13, 2013, 8:20 p.m. UTC | #6
On Tuesday 13 of August 2013 21:18:16 Mark Brown wrote:
> On Tue, Aug 13, 2013 at 07:55:47PM +0100, Mark Brown wrote:
> > I'm still debugging what's going on here - the basic refcounting all
> > looks OK in the SPI driver, I can see it requesting and releasing with
> > the refcounts all going back to zero in the DMA driver as expected but
> > when we come back to the device later on dmaengine is deciding the
> > device is unavailable quite early on in the process.
> 
> The failure is happening because this check is failing:
> 
> 	/* devices with multiple channels need special handling as we need 
to
> 	 * ensure that all channels are either private or public.
> 	 */
> 	if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask))
> 		list_for_each_entry(chan, &dev->channels, device_node) {
> 			/* some channels are already publicly allocated */
> 			if (chan->client_count) {
> 
> which is happening because dma1chan0 (which is on the same DMA
> controller as the SPI controller) and in fact every other DMA channel
> had references grabbed by the network stack dmaengine helpers which I'd
> enabled in config.  The fact that they do that is unhelpful, it renders
> the API mostly useless, but is nothing to do with this series.
> 
> Having tweaked the config everything appears to work so:
> 
> Tested-by: Mark Brown <broonie@linaro.org>
> 
> though the whole thing with the filter function is as I say a bit fun
> from a code review point of view.

I believe you just found another brokenness of current DMA channel 
matching.

Thanks for testing.

Best regards,
Tomasz
Mark Brown Aug. 13, 2013, 8:29 p.m. UTC | #7
On Tue, Aug 13, 2013 at 10:20:19PM +0200, Tomasz Figa wrote:
> On Tuesday 13 of August 2013 21:18:16 Mark Brown wrote:

> > 	if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask))
> > 		list_for_each_entry(chan, &dev->channels, device_node) {
> > 			/* some channels are already publicly allocated */
> > 			if (chan->client_count) {

> > which is happening because dma1chan0 (which is on the same DMA
> > controller as the SPI controller) and in fact every other DMA channel
> > had references grabbed by the network stack dmaengine helpers which I'd
> > enabled in config.  The fact that they do that is unhelpful, it renders
> > the API mostly useless, but is nothing to do with this series.

> I believe you just found another brokenness of current DMA channel 
> matching.

Yeah, at first glance the reference grabbing thing seems a bit peculiar.
I didn't research why it's done that way yet.
diff mbox

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 89cbbab..241a049 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -366,7 +366,7 @@  config SPI_S3C24XX_FIQ
 config SPI_S3C64XX
 	tristate "Samsung S3C64XX series type SPI"
 	depends on (ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5P64X0 || ARCH_EXYNOS)
-	select S3C64XX_DMA if ARCH_S3C64XX
+	select S3C64XX_DMA if ARCH_S3C64XX && !S3C64XX_PL080
 	help
 	  SPI driver for Samsung S3C64XX and newer SoCs.