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

SPI DMA for RPi 4 doesnt work #3570

Open
lucastanure opened this issue Apr 23, 2020 · 13 comments
Open

SPI DMA for RPi 4 doesnt work #3570

lucastanure opened this issue Apr 23, 2020 · 13 comments

Comments

@lucastanure
Copy link
Contributor

Data for SPI transfer bigger than 96 Bytes are corrupted. In my test, if I send 20480 bytes in sequence (0x00 0x01 0x02...) the SPI Bus sends corrupted data, sometimes 0xFF for a few chunks, 0x00 for other chunks and random data too.
But If disable DMA by returning false always from bcm2835_spi_can_dma, everything works.

The same kernel 4.19 works fine at RPi3.

To reproduce
Use SPI dev to send larger data over SPI using Rpi4 Only.
Check the data using Saleae logic 16.

Expected behaviour
No corrupted data in SPI bus.

Actual behaviour
Using Saleae logic 16 I can see corrupted data in the bus. Is not a hardware issue because disabling DMA everything works.

System
https://pastebin.com/iaWNH4v8

  • Which model of Raspberry Pi?
    RPi 4, 4Gb

  • Which OS and version (cat /etc/rpi-issue)?
    Generated using pi-gen, https://github.com/RPi-Distro/pi-gen, 5f884374b6ac6e155330c58caa1fb7249b8badf1, stage2

  • Which firmware version (vcgencmd version)?
    Feb 12 2020 12:36:21
    Copyright (c) 2012 Broadcom
    version c3c8dbdf147686fb0c3f32aece709d0653368810 (clean) (release) (start)

  • Which kernel version (uname -a)?
    Linux raspberrypi 4.19.113-v7l+ framebuffer code suggestions #4 SMP Thu Apr 23 08:31:14 BST 2020 armv7l GNU/Linux

@pelwell
Copy link
Contributor

pelwell commented Apr 23, 2020

I have a Logic Pro 16 and the software - can you provide a test application and capture files showing the corruption?

@lucastanure
Copy link
Contributor Author

Hi,

Right now I'm using an SPI device with firmware, but I can't share that.
I'm working to reproduce just using SPI DEV.

@lucastanure
Copy link
Contributor Author

Hi,

Please use https://pastebin.com/EgpDw0YR driver and device tree to reproduce the issue.

Left is the bad transfer , right is the good transfer
image

@lucastanure
Copy link
Contributor Author

The problem is because request_firmware gives me vmalloc memory and for some reason, the translation to continuos memory fails on RPI4.

@lucastanure
Copy link
Contributor Author

Same kernel and test on RPI3

image

@pelwell
Copy link
Contributor

pelwell commented Apr 23, 2020

Thanks - the test is running on Pi 3 and Pi 4 (and no longer crashing once I fixed the buffer freeing...) and demonstrating the problem on Pi 4. It first appeared to be working on both, but the first batch of transmitted data, although not zero, wasn't the counting sequence.

@pelwell
Copy link
Contributor

pelwell commented Apr 27, 2020

On Pi 4, not all RAM may be addressable by the DMA controller. There are some new 40-bit DMA channels which could be used, but they have restrictions and haven't been enabled for general use yet.

However, the kernel SPI documetation (https://www.kernel.org/doc/Documentation/spi/spi-summary) says:

Follow standard kernel rules, and provide DMA-safe buffers in
your messages. That way controller drivers using DMA aren't forced
to make extra copies unless the hardware requires it (e.g. working
around hardware errata that force the use of bounce buffering).

If standard dma_map_single() handling of these buffers is inappropriate,
you can use spi_message.is_dma_mapped to tell the controller driver
that you've already provided the relevant DMA addresses.

In other words, you'll get the best performance using dma_alloc_coherent to allocate your buffer, because it is guaranteed to be DMA-able.

@lucastanure
Copy link
Contributor Author

On Pi 4, not all RAM may be addressable by the DMA controller.

So that means that even if I do dma_alloc_coherent, I can get some failures some times because the memory was allocated out of boundary for Rpi4.

In other words, you'll get the best performance using dma_alloc_coherent to allocate your buffer, because it is guaranteed to be DMA-able.

Well, that means that I have to change how the driver works only because of Rpi4?
Doesn't seems to be the proper way to go to change all the drivers that just request firmware and send it through SPI to add some more steps just because of RPI4.
In other words, can't just use request_firmware, I will need to or memcpy the firmware to a new location or request_firmware_into_buf with some extra steps, and all of this may not even work because of Rpi4 not able to access all the memory.

And not just request_firmware, must a lot of areas of the kernel that just send virtual memory using DMA. How to deal with all the other areas?

@pelwell
Copy link
Contributor

pelwell commented Apr 27, 2020

So that means that even if I do dma_alloc_coherent, I can get some failures some times because the memory was allocated out of boundary for Rpi4.

No - dma_alloc_coherent will allocate memory in area which is DMA safe.

Well, that means that I have to change how the driver works only because of Rpi4?

You aren't following best practises, but I accept that it does work for you on other platforms. I will have a look at making use of a 40-bit DMA channel, but a small wrapper function that memcpys into contiguous, DMA-reachable memory won't take much code and will have negligible performance impact.

Or just set total_mem=1024 in config.txt and the problem will go away.

@lucastanure
Copy link
Contributor Author

lucastanure commented Apr 27, 2020

You aren't following best practises

Can you give more details?
because its request_firmware that allocates the memory.

What is the best practice?

@pelwell
Copy link
Contributor

pelwell commented Apr 27, 2020

Follow standard kernel rules, and provide DMA-safe buffers in your messages.

Allocate some memory using dma_alloc_coherent, then use firmware_request_into_buf to load it?

@pelwell
Copy link
Contributor

pelwell commented Apr 30, 2020

I've just pushed two commits to the rpi-4.19.y branch that fix some bugs in the 40-bit DMA support and allow the use of 40-bit DMA for SPI (17cba8a). There are only a 3 or 4 40-bit DMA channels on 2711, and enabling them on SPI claims 2 of them; for that reason I've made it an opt-in change - put dtparam=spi_dma4 in config.txt.

Needless to say, you'll need to pull in the latest commits and rebuild your kernel. You'll also need a fairly new firmware due to the use of some new DT parameter syntax - anything released this year (including the current Raspbian image) is fine.

Thanks for creating this issue - your test app has been very useful in diagnosing the bug in the DMA driver.

@lucastanure
Copy link
Contributor Author

Thanks!

popcornmix added a commit to raspberrypi/firmware that referenced this issue May 12, 2020
kernel: vc4_hdmi: Add CEC support for 2711
See: raspberrypi/linux#3601

kernel: overlays: Move fixed-clock nodes to the root
See: raspberrypi/linux#3607

kernel: bcm2835_isp fixes for constness
See: raspberrypi/linux#3592

kernel: video: bcm2708_fb: Disable FB if no displays found
See: raspberrypi/linux#3598

kernel: vc4_hdmi_phy: Fix typo in phy_get_cp_current
See: raspberrypi/linux#3594

kernel: configs: Add missing TOUCHSCREEN_RASPBERRYPI_FW=m
See: Hexxeh/rpi-firmware#223

kernel: configs: Add missing PPS configs
See: raspberrypi/linux#3593

kernel: overlays: gpio-keys: Avoid open-drain warnings
See: #1381

kernel: overlays: Make the i2c-gpio overlay safe again

kernel: ARM: dts: bcm2711: Allow 40-bit DMA for SPI
See: raspberrypi/linux#3570

firmware: In KMS mode, prevent use of the firmware rotations

firmware: power: Adjust ARM:AXI divider ratio if ARM freq > 1500 MHz

firmware: imx477: Remove STILLS flag from 720p mode

userland: tvservice: Fix freeze on old firmware
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this issue May 12, 2020
kernel: vc4_hdmi: Add CEC support for 2711
See: raspberrypi/linux#3601

kernel: overlays: Move fixed-clock nodes to the root
See: raspberrypi/linux#3607

kernel: bcm2835_isp fixes for constness
See: raspberrypi/linux#3592

kernel: video: bcm2708_fb: Disable FB if no displays found
See: raspberrypi/linux#3598

kernel: vc4_hdmi_phy: Fix typo in phy_get_cp_current
See: raspberrypi/linux#3594

kernel: configs: Add missing TOUCHSCREEN_RASPBERRYPI_FW=m
See: #223

kernel: configs: Add missing PPS configs
See: raspberrypi/linux#3593

kernel: overlays: gpio-keys: Avoid open-drain warnings
See: raspberrypi/firmware#1381

kernel: overlays: Make the i2c-gpio overlay safe again

kernel: ARM: dts: bcm2711: Allow 40-bit DMA for SPI
See: raspberrypi/linux#3570

firmware: In KMS mode, prevent use of the firmware rotations

firmware: power: Adjust ARM:AXI divider ratio if ARM freq > 1500 MHz

firmware: imx477: Remove STILLS flag from 720p mode

userland: tvservice: Fix freeze on old firmware
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

No branches or pull requests

2 participants