Details

Reviewers

imp
pkelsey

Group Reviewers

riscv
arm64

Commits

rG253c83058deb: mmc: SPI-mode support for SD cards.

Summary

Add SPI mode support for the cases when SD card socket is connected directly to a SPI bus.

This might be useful for low end or RISC-V research systems when they lack an MMC controller in the SoC, examples are Codasip, lowRISC, CVA6, etc

Project timeline:
2007: Warner mentioned SPI operational mode in his MMC talk: https://people.freebsd.org/~imp/bsdcan2007.pdf
2012: Patrick Kelsey has engineered the support
2025: Ruslan cleaned up, fixed bugs and tested on Codasip platform (FPGA) with Xilinx SPI

Test Plan

Tested on Codasip A730, but also will test on CVA6

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

br requested review of this revision.Mar 5 2025, 4:46 PM

br created this revision.

br added reviewers: pkelsey, arm64.Mar 14 2025, 11:41 AM

@br Interesting to see this being revived. It has been quite some time since I wrote the base version of this code. Are you able to provide some description of the "fixed bugs" you noted in the summary?

The hardware I used to originally develop this is long gone (and was mips32 - not supported anymore anyway), so I cannot offer regression testing. I also have not been paying attention to any intervening evolution in the interfacing subsystems (newbus, mmc, spi...), so at this point I think I will be limited to commentary on the code documentation.

pkelsey added inline comments.Mar 15 2025, 3:45 AM

sys/dev/mmc/mmcspi.c
75	Update this comment as the list of devices that appeared in the original code has been removed (and I see no reason to bring it back).
1335–1338	Clarified this comment, as part of it seemed to be aimed at an earlier version of the code and was confusing in the current context.

Fix comments per @pkelsey request

br added inline comments.Apr 3 2025, 10:14 AM

sys/dev/mmc/mmcspi.c
1737–1738	@pkelsey this else statement block was added to you original code. This translates R7 response to R1, otherwise does not detect card

pkelsey added inline comments.Apr 3 2025, 10:05 PM

sys/dev/mmc/mmcspi.c
1737–1738	@br Thanks for calling my attention to this change. This raises the question as to why it was necessary to make this change, and the additional question of why it seems on the face of it to not make any sense (Only the SEND_IF_COND command uses R7, and it does so in both SPI and SD mode, so there should be no need for a conversion from SPI R7 to SD R1, which is what this change appears to be). The reason you found the need to make this change is commit 6e0628d43299c2c10a7425cf13614e82d0304f22. Before that commit the MMC_RSP_{R1, R1B, R2, R3, R6/R7} values were distinct, and after it, MMC_RSP_{R6/R7} became equal to MMC_RSP_R1. This code was developed before that commit and relied on the prior distinctiveness of those values. Even with your change, the else if block starting at line 1809 is dead, leading to a different latent bug. What your change here is doing, contrary to surface appearances, is catching the SEND_IF_COND case and performing an SPI R7 to SD R7 (not R1) conversion, as you are overwriting mmc_cmd->resp[0] with the SD R7 format (which the command initiator will be expecting). The proper fix is to remove this hack, and replace the use of MMC_RSP() with a distinct response classification that is properly determined in setup_command().

mmcspi.c55 KBDownload

I've uploaded a version of this file that removes the R7 translation hack and fixes the underlying issue as I've proposed. I did not compile this code, so please excuse any mechanical issues that you find. The SPI response type to SD/MMC response type correspondence that has been added here corresponds to what the code implemented prior to the commit I referenced above, and I double checked it for each command against the current version of the SD Physical Layer Simplified Specification.

In D49248#1132215, @pkelsey wrote:

mmcspi.c55 KBDownload

I've uploaded a version of this file that removes the R7 translation hack and fixes the underlying issue as I've proposed. I did not compile this code, so please excuse any mechanical issues that you find. The SPI response type to SD/MMC response type correspondence that has been added here corresponds to what the code implemented prior to the commit I referenced above, and I double checked it for each command against the current version of the SD Physical Layer Simplified Specification.

@pkelsey Could you add permission to the file uploaded, so I can download.

@pkelsey Could you add permission to the file uploaded, so I can download.

@br I wasn't aware of that - I think it's resolved now.

In D49248#1132271, @pkelsey wrote:

@pkelsey Could you add permission to the file uploaded, so I can download.

@br I wasn't aware of that - I think it's resolved now.

It does detect the card with your patch, but size is 0bytes (CSD is zero)

mmc0: New card detected (CID 035344534236344780da29024a014400)
mmc0: New card detected (CSD 00000200000000000000000000000000)

I am looking if I can find an issue...

It does detect the card with your patch, but size is 0bytes (CSD is zero)
mmc0: New card detected (CID 035344534236344780da29024a014400)
mmc0: New card detected (CSD 00000200000000000000000000000000)
I am looking if I can find an issue...

I checked all of the response type mappings against the spec again and found a mistake where I had indexed into the wrong table for the SEND_CSD and SEND_CID translations when making this patch. Attached is a new version that corrects those mappings (as well as corrects another mapping for an SPI-only command, although that change has no functional effect). I expect this will sort out the issue you are observing.

mmcspi.c55 KBDownload

pkelsey attached a referenced file: F114031984: mmcspi.c. (Show Details)Apr 7 2025, 3:38 AM

In D49248#1132893, @pkelsey wrote:
It does detect the card with your patch, but size is 0bytes (CSD is zero)
mmc0: New card detected (CID 035344534236344780da29024a014400)
mmc0: New card detected (CSD 00000200000000000000000000000000)
I am looking if I can find an issue...
I checked all of the response type mappings against the spec again and found a mistake where I had indexed into the wrong table for the SEND_CSD and SEND_CID translations when making this patch. Attached is a new version that corrects those mappings (as well as corrects another mapping for an SPI-only command, although that change has no functional effect). I expect this will sort out the issue you are observing.

mmcspi.c55 KBDownload

This one works great! Thank you :)

the hack around MMC return code is now resolved by @pkelsey

pkelsey added inline comments.Apr 7 2025, 5:53 PM

sys/dev/mmc/mmcspi.c
1160	@br This looks like a bug to me - the predicate should be the opposite sense of what is written, in other words: if (!mmcspi_cmd->use_crc) Otherwise, the code that employs `error_mask` when processing SPI command responses will fail to detect CRC errors when CRC checking is enabled and they occur.

Fix error mask settings when CRC is disabled

pkelsey accepted this revision.Apr 8 2025, 7:30 PM

This revision is now accepted and ready to land.Apr 8 2025, 7:30 PM

Closed by commit rG253c83058deb: mmc: SPI-mode support for SD cards. (authored by br). · Explain WhyApr 9 2025, 11:23 AM

This revision was automatically updated to reflect the committed changes.

br added a commit: rG253c83058deb: mmc: SPI-mode support for SD cards..

mmc: SPI mode support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 151920

sys/conf/files

sys/dev/mmc/mmcspi.c

mmc: SPI mode supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 151920

sys/conf/files

sys/dev/mmc/mmcspi.c

mmc: SPI mode support
ClosedPublic
Actions

Revision Contents
Changeset List