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

Introduce SDMMC write_blocks #453

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conversation

phoracek
Copy link
Contributor

@phoracek phoracek commented Sep 18, 2023

write_block needs to be called for every 512 bytes long block that is to be written to the SD card. Each of these calls requires the overhead of negotiating the write command and if I understand it correctly it also requires to erase the whole SD segment and copying it back.

Speed

With this patch, write_blocks is introduced, mirroring the behavior of read_block and read_blocks. This new method by itself helped me to improve write speed from the original example of 9.2 kB/s to 65 kB/s.

Write performance with 480 MHz system clock, 50 MHz bus speed, and enabled caches:

Buffer size Total size written Speed
64 kB 64 kB 513 kB/s
64 kB 32 MB 4.9 MB/s
128 kB 32 MB 5.2 MB/s

Erase before writing

Using ACMD23 to erase block data before writing in my case did not improve the result. Either my card does not benefit from it or I used it wrong.

Possible improvements

Although the results look good Class 10 SD cards should be much faster. Even with half the bus speed, it should achieve at least 10 MB/s. If anybody has an idea what may be the bottleneck, please share.

Follow ups

The read check is processing a buffer 10 blocks long. The check then
reads data from the SD card 10 times to then report the speed of
"Read 10 blocks at X bytes/second". The problem is that it actually
reads 10*10 blocks.

With this patch, read_blocks is used correctly to populate a continuous
buffer.

Signed-off-by: Petr Horacek <[email protected]>
@phoracek phoracek changed the title Write blocks Introduce SDMMC write_blocks Sep 18, 2023
@phoracek phoracek marked this pull request as draft September 18, 2023 17:10
@phoracek phoracek force-pushed the write_blocks branch 3 times, most recently from c41dabd to 117f43a Compare September 18, 2023 21:14
@phoracek phoracek marked this pull request as ready for review September 18, 2023 21:15
@phoracek phoracek marked this pull request as draft September 19, 2023 16:21
@ost-ing
Copy link
Contributor

ost-ing commented Sep 19, 2023

Strange, we seem to have come up with a PR to solve this at the same time.

I use CMD23 to set the SD block length for the transfer, which may help, see #455

@phoracek
Copy link
Contributor Author

phoracek commented Sep 19, 2023

@ostenning ha! 😁

Interesting, I did not know CMD23 can be used instead of CMD12 for SD.

I tried using CMD23 from your patch in my benchmark instead of stop_transmission. CMD23 was a little bit slower (4.9 MB/s instead of 5.3 MB/s), but that may have been just my specific card or the benchmark config.

@phoracek phoracek marked this pull request as ready for review September 19, 2023 18:20
@ost-ing
Copy link
Contributor

ost-ing commented Sep 19, 2023

@ostenning ha! 😁

Interesting, I did not know CMD23 can be used instead of CMD12 for SD.

I tried using CMD23 from your patch in my benchmark instead of stop_transmission. CMD23 was a little bit slower (4.9 MB/s instead of 5.3 MB/s), but that may have been just my specific card or the benchmark config.

I'm sure @richardeoin will have greater insights

@phoracek
Copy link
Contributor Author

Thanks @ostenning.

I have posted a draft of this new method being used in FAT writes. It still needs some care, so I keep it in a separate PR: #456

`write_block` needs to be called for every 512 bytes long block that is
to be written to the SD card. Each of these calls requires the overhead
of negotiating the write command.

With this patch, `write_blocks` is introduced, mirroring the behavior
of `read_block` and `read_blocks`. This new method helped me to improve
write speed of the original 9492 kB/s to 66252 kB/s.

Signed-off-by: Petr Horacek <[email protected]>
@richardeoin
Copy link
Member

Great! I don't have any specific insights, other than that this looks like a nice PR.

It's possible that some cards use CMD23 to optimise writes, but here I like the simplicity of having write_blocks as similar to write_block as possible.

It looks like this PR is ready, so I'll go ahead and merge it

@richardeoin richardeoin added this pull request to the merge queue Sep 19, 2023
Merged via the queue into stm32-rs:master with commit 600969f Sep 19, 2023
22 checks passed
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.

3 participants