-
Notifications
You must be signed in to change notification settings - Fork 240
Fix alignment for DMA on STM32H7 in function mmcsd_read_csd() #347
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
base: px4_firmware_nuttx-10.3.0+
Are you sure you want to change the base?
Fix alignment for DMA on STM32H7 in function mmcsd_read_csd() #347
Conversation
This function gets called during init of an MMC card. STM32H7 DMA expects 32 byte alignment, so the call to SDIO_DMAPREFLIGHT() fails.
farhangnaderi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with PX4? which PX4 you are testing this with?
We have a custom PX4 board based on V6X, and we have a custom fork of PX4 1.15. I tested it only on that. If someone has a standard PX4 drone with a microSD, it can be tested if they also have an EMMC adapter card. We are planning on moving from the removable SD to a soldered-on EMMC so that's why I am testing with the EMMC adapter card. Once I made the changes, the EMMC card mounts fine and works 100% like a normal SD as far as PX4 is concerned, storing data, logs, etc. |
is this the Firelfy board with microSD on it? I can test it maybe. |
It is for Firefly Drone Shows, not Freefly or whatever the similar one is. Only we have these boards. But it should be testable with any PX4 compatible board with STM32H7 and a micro SD. I did search for PX4 with EMMC and there are not many hits so I don't think many or any boards are using EMMC yet. We are switching because we have so many drones and SD cards and we don't really remove them ever, just pull logs from QGC. |
Yup, I also mentioned Firefly! 🙂 |
dakejahl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me but let's see if @davids5 can comment
davids5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What SDMMC are you using 1 or 2 on your HW?
CONFIG_STM32H7_SDMMC2 |
davids5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have several issues with the change and how it comes in. Since the normal process is to upstream changes and then backport. This change would need to follow that process. Any change in this area needs to be HW tested on many targets.
The change in this PR is in an non-arch specific for an arch specific reason. It potentially will cause a stack size increase of 16 bytes. This may cause other issues on other targets and other archs.
The H7 SDMMC uses IDMA that has 32 bit alignment requirement. The arch (stm32_sdmmc.c) driver has workarounds for unaligned access. But since the H7 SDMMC driver was written the dcache code was updated in NuttX to supports un-cache line aligned operations. (it does a clean on the fragments) So the alignment check portion in the preflight code may not be needed any longer (on SDMMC2).
First I would investigate removing https://github.com/apache/nuttx/blob/master/arch/arm/src/stm32h7/stm32_sdmmc.c#L3095-L3112
If that solves the issue in the arch specific driver it has no implication for other targets. That change could be roundtripped and have the least impact. If it does not work I would proceed with the second phase below:
The upstream mmcsd has diverged a lot in the csd access code (it does excsd reading now) from the branch point we are on.
I would attempt a backport of the upstream mmcsd driver and see if it can come in with minimal changes (semaphore usage etc). Then see if it solves the issue for you. If not I would add a Kconfig default that defaults to 16 byte alignment and can be overridden in an arch that needs it.
Use syslog() to print because regular debug printing with finfo() is disabled in build.
This function (mmcsd_read_csd()) gets called during init of an MMC card. STM32H7 DMA expects 32 byte alignment, so the call to SDIO_DMAPREFLIGHT() fails.
Summary
Issue #346
Impact
Testing