Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use bcm2835-dma #1153

Merged
merged 7 commits into from
Oct 9, 2015
Merged

Use bcm2835-dma #1153

merged 7 commits into from
Oct 9, 2015

Conversation

notro
Copy link
Contributor

@notro notro commented Oct 4, 2015

This PR patches up bcm2835-dma to match bcm2708-dmaengine and uses it when booting with Device Tree. Non-DT booting continues to use bcm2708-dmaengine.
bcm2835-dma exposes the legacy DMA API by using bcm2708-dmaengine.

Tested on Pi1 and Pi2 with and without Device Tree + ARCH_BCM2835.

7 DMA channels:

$ ls /sys/class/dma
dma0chan0  dma0chan1  dma0chan2  dma0chan3  dma0chan4  dma0chan5  dma0chan6

Relevant boot log entries with Device Tree:

[    0.746285] bcm2835-dma 3f007000.dma: DMA legacy API manager at f3007000, dmachans=0x1
[    0.755260] bcm2835-dma 3f007000.dma: dma_debug:0
...
[    1.052264] BCM2708FB: allocated DMA memory fac00000
[    1.057290] BCM2708FB: allocated DMA channel 0 @ f3007000
...
[    1.872134] mmc-bcm2835 3f300000.mmc: mmc_debug:0 mmc_debug2:0
[    1.882626] mmc-bcm2835 3f300000.mmc: DMA channels allocated

Without DT (the channel 13 error also occurs without this PR):

[    0.726604] bcm2708-dmaengine bcm2708-dmaengine: DMA legacy API manager at f3007000, dmachans=0x7f35
[    0.735883] bcm2708-dmaengine bcm2708-dmaengine: failed to get irq for DMA channel 13
[    0.743759] bcm2708-dmaengine bcm2708-dmaengine: Initialized 8 DMA channels (+ 1 legacy)
[    0.752656] bcm2708-dmaengine bcm2708-dmaengine: Load BCM2835 DMA engine driver
[    0.760010] bcm2708-dmaengine bcm2708-dmaengine: dma_debug:0
...
[    1.054439] BCM2708FB: allocated DMA memory fac00000
[    1.059465] BCM2708FB: allocated DMA channel 0 @ f3007000
...
[    1.872010] mmc-bcm2835 mmc-bcm2835.0: mmc_debug:0 mmc_debug2:0
[    1.882528] mmc-bcm2835 mmc-bcm2835.0: DMA channels allocated

bcm2708_fb uses the legacy DMA API, so in order to start using
bcm2835-dma, bcm2835-dma has to support the legacy API. Make this
possible by exporting bcm_dmaman_probe() and bcm_dmaman_remove().

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Add slave transfer capability to BCM2835 dmaengine driver.
This patch is pulled from the bcm2708-dmaengine driver in the
Raspberry Pi repo. The work was done by Gellert Weisz.

Tested using the bcm2835-mmc driver from the same repo.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
bcm2835-dma supports residue reporting at burst level but didn't report
this via the residue_granularity field.

Without this field set properly we get playback issues with I2S cards.

[by HiassofT, taken from bcm2708-dmaengine]
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Load driver early since at least bcm2708_fb doesn't support deferred
probing and even if it did, we don't want the video driver deferred.
Support the legacy DMA API which is needed by bcm2708_fb.
Don't mask out channel 2.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
[by popcornmix, taken from bcm2708-dmaengine]
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Start to use bcm2835-dma when booting with Device Tree.
bcm2708-dmaengine will be used with non-DT boot.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Both bcm2835-dma and bcm2708-dmaengine have the same compatible string.
So change compatible to "brcm,bcm2708-dma".

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
@popcornmix
Copy link
Collaborator

Looks reasonable to me. We could probably drop the debug wait states commit as so far it hasn't fixed any problematic sdcards (although it's self contained and could be dropped later).

Also I think non-DT support is just waiting for an excuse to drop it. I don't think you'd have any objections if you stopped supporting it (on 4.2 or later trees).

@notro
Copy link
Contributor Author

notro commented Oct 5, 2015

Also I think non-DT support is just waiting for an excuse to drop it.

Actually I'm working on patches that will remove non-DT support where ever I can find it. Primarly just to see how much I can remove. I can make it into a PR so we can discuss it.

@popcornmix
Copy link
Collaborator

Yes. I'm sure non-DT will be removed, it's just when.

@pelwell
Copy link
Contributor

pelwell commented Oct 6, 2015

With the latest changes, bcm2835-dma.c and bcm2708-dmaengine,c are so similar that it is easier to compare the two files than it is to review the changes. If it weren't for a large number of mainly pointless cosmetic differences then bcm2835-dma.c would almost be a large subset of the downstream driver.

No objections.

popcornmix added a commit that referenced this pull request Oct 9, 2015
@popcornmix popcornmix merged commit 3d6223c into raspberrypi:rpi-4.2.y Oct 9, 2015
@popcornmix
Copy link
Collaborator

Thanks. Seemed to work fine in a run time test.

@HiassofT
Copy link
Contributor

I've noticed 2 differences compared to bcm2708-dmaengine:

bcm2835_dma_slave_config doesn't set c->dreq to cfg->slave_id if c->dreq is 0.

This causes I2S soundcards to break. You can reproduce it with the rpi-dac overlay (no hardware needed), then run aplay or speaker-test and you'll get a lot of underruns instead of sound output.

Adding the missing code fixes this issue

--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -633,6 +633,8 @@ static int bcm2835_dma_slave_config(struct dma_chan *chan,
        }

        c->cfg = *cfg;
+       if (!c->dreq)
+               c->dreq = cfg->slave_id;

        return 0;
 }

Second issue: with MAX_LITE_TRANSFER set to SZ_64K-4 (instead of 32k as in the bcm2708 code) I get repeating clicks as soon as period_len is greater than 32k. i.e. with the default alsa settings aplay with a 48kHz/16bit file plays fine, but for example 48kHz/32bit or 96kHz/16bit files click.

Setting MAX_LITE_TRANSFER to SZ_32K fixes this issue. Not sure though if alsa or the DMA controller is to blame for that.

Other than that: great job, with these 2 fixes everything seems to be working really fine!

@notro
Copy link
Contributor Author

notro commented Oct 10, 2015

bcm2835_dma_slave_config doesn't set c->dreq to cfg->slave_id if c->dreq is 0.

Yes, the driver expects the channel to come from Device Tree which sets dreq.
But we're not there yet, so this change is needed.
The patch was initially made for mainline, so this slipped through the cracks. Good catch.

Setting MAX_LITE_TRANSFER to SZ_32K fixes this issue.

I just tested spi-bcm2835 with a 65532 byte buffer on a SPI display and it worked fine.
So changing it would have to be for the cyclic dma case only.

Mainline bcm2835-dma doesn't check the transfer size at all and it is used with bcm2835-i2s. But I don't know how well it is tested with regards to buffer sizes. And I don't know if it was tested with normal or lite channels.

The easy fix is to just set it to 32k for cyclic dma, unless @popcornmix or @pelwell has objections or insight into why this happens.

I2S is one a the few areas where we haven't switched to the mainline driver, hopefully someone will look into this.

@clivem
Copy link

clivem commented Oct 11, 2015

Until someone does have time to look into making the mainline dma driver work properly with I2S, can we please revert this commit from the 4.2.y tree? I would suggest that the 4.3.y tree would be a better "playground" for this sort of change.

@HiassofT
Copy link
Contributor

@notro Thanks a lot for the detailled explanation!

Limiting only cyclic transfers looks like a good solution. I've created PR #1160 with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants