You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/03/12 14:40:16 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
davids5 opened a new pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047
## Summary
mmcsd:Remove CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
stm32h7:sdmmc remove CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
stm32f7:sdmmc remove CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
stm32f7:sdmmc WRITE COMPLETE prevent false triggers
stm32h7:sdmmc WRITE COMPLETE prevent false triggers
While testing PR #2989 on the H7 I noticed that the cards
were staying in 1-bit mode. The root cause was that the
scr read path was using DMA without an invlidate.
This was caused by CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT,
but the sdmmc driver, did not use the delayed invalidate
nor would it work on 8 bytes.
The driver fully supported dcache mgt on runt buffers, but
the #ifdef CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT blocked it.
Reviewing the PR that added CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
it may have been valid at the time. But after the dcache operations
we fixed. It is not necessary and offers no benefit.
## Impact
STM32H7 SD card performances is ~1/4 of what is should be.
SIDO Interfaces is not arch dependent.
## Testing
H7 - px4_fmu-v6x
F7 - px4_fmu-v5x
Dcache off, Write Through , Write Back
sd_bench
cat large file with know pattern
I also tested the write completion interrupt timing, and found some false triggers. That then cause waits. This has been fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-797559092
@davids5 Looks good to me, except I still think we need to protect for the case where the GPIO doesn't get configured for interrupt until after the interrupt fired. The current code would work fine in the case that you call eventwait right away, but a write followed by a long pause will result in a timeout rather than a WRCOMPLETE in that case. I agree with you that the timing should never really allow you to have this happen, but it's probably best to catch it anyway.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino edited a comment on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
antmerlino edited a comment on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-797559092
@davids5 Looks good to me, except I still think we need to protect for the case where the GPIO doesn't get configured for interrupt until after the interrupt fired. The current code would work fine in the case that you call eventwait right away, but a write followed by a long pause will result in a timeout rather than a WRCOMPLETE in that case. I agree with you that the timing should never really allow you to have this happen, but it's probably best to catch it anyway.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-799030922
> @antmerlino this code here handle the late case. I do need to put this on top of your PR and test again. If we bring this in first the I can retest your PR rebased or visa-versa.
>
> https://github.com/apache/incubator-nuttx/blob/cf9241b28d7f93584e830d87892c13b81a38f028/arch/arm/src/stm32f7/stm32_sdmmc.c#L2843
I agree that this takes care of the case when you perform another operation right after a write, but what happens when you write and then don't come back in to call eventwait for a long time? If you got the interrupt enabled in time, you're covered. But if you didn't get the interrupt enabled in time and you don't perform another operation soon after writing, you won't call eventwait to check the pin and therefore will timeout, despite the write being complete.
So I do think we need additional logic inside WAITENABLE to catch this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-797539793
FYI @jlaitine. I have asked Bob F to review this as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-797716774
> @davids5 Looks good to me, except I still think we need to protect for the case where the GPIO doesn't get configured for interrupt until after the interrupt fired. The current code would work fine in the case that you call eventwait right away, but a write followed by a long pause will result in a timeout rather than a WRCOMPLETE in that case. I agree with you that the timing should never really allow you to have this happen, but it's probably best to catch it anyway.
@antmerlino this code here handle the late case. I do need to put this on top of your PR and test again. If we bring this in first the I can retest your PR rebased or visa-versa.
https://github.com/apache/incubator-nuttx/blob/cf9241b28d7f93584e830d87892c13b81a38f028/arch/arm/src/stm32f7/stm32_sdmmc.c#L2843
the Writcomplete logic takes the wait from 11-16+ mS down 9us 13 uS max.
ΔT 10.124115664 s
Nfalling 5.376 k
Nrising 5.377 k
fmin 23.702 Hz
fmax 1.25 kHz
fmean 535.869 Hz
Tstd 2.177 ms
Hmin 790.406 µs
Hmean 1.855 ms
HSDev 2.174 ms
Hmax 42.182 ms
Lmin 8.969 µs
Lmean 9.42 µs
LSDev 281.8 ns
**Lmax 13.625 µs**
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #3047: mmcsd:Stuck in 1-bit mode, Removed CONFIG_ARCH_HAVE_SDIO_DELAYED_INVLDT
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #3047:
URL: https://github.com/apache/incubator-nuttx/pull/3047#issuecomment-797642264
I agree that delayed invalidate doesn't really give any benefit; I am not sure I even really ever understood the motivation behind it. I fully support removing it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org