-
Notifications
You must be signed in to change notification settings - Fork 17
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
SD card access and Read Only FAT32 file system support #360
Conversation
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.
Thanks for putting this together. I just left a few comments while reading through the code. We'll need to get CI to pass before merging, but this is good enough to share around to people that want to get started with SD cards.
Thanks for this @alees24. I'm keen we do not spend significant time on this right now so let's not worry about spending time on an SD card DPI model. As we already have the test code it's reasonable for us to include the test in CI to run against the real FPGA. We also need to consider how to to best use this in sonata-software. It's important we have a working example in there, so where shall We could also just say no sd card util at all in sonata-system to simplify things. That would mean no testing of sd card functionality in CI though. I don't think it's that important as it's just a plain SPI device but would mean if we mess up something do with the pinmux/pin mapping it would get flagged. |
The DPI model already exists and works; sdcard_tests.hh works in sim and it's even possible to play video files to the simulated LCD; that was done a couple of weeks ago. I guess it wasn't clear that I meant only that it needs a little tidying before inclusion, so it's not in this PR. |
Awesome. May as well leverage it then. re: example in sonata-software I think the thing to do is get this PR merged ignoring that issue. We can then deal with that separately. |
08104a1
to
19c130a
Compare
Right, I think this is ready to go; long filename support and directory traversal/matching could use some more testing but it works well enough for our present needs. The DPI model is now present, and although the FPGA system presently has problems, 'test_runner' should pass on this PR when they have been resolved. For simulation, however, we have an issue.....it needs an 'sd.img' file that is ca. 1GiB (see the instructions I've added in 'doc/guide/sdcard-setup.md') which currently won't be present when the simulation runs, presumably....suggestions welcome, please! |
5ece601
to
db37f73
Compare
I see a few possible options for resolving the simulator situation:
Opinions/thoughts, please? |
If we need to go done this route, we can use git large file storage.
My initial though is that this is best. Would you be able to have a go at this?
See my opinions inline. |
db37f73
to
062bbb6
Compare
062bbb6
to
57db43f
Compare
Note that this manual test will corrupt the contents of the card so it can only be run manually and will intentionally insist that you wilfully insert an SD card before it performs any writes to the card. This is to avoid inadvertent data loss. SD card layer can perform initialisation, CID/CSD reading, and single/multiple block read/write transfers from/to a number of microSD cards used in testing. Only SDHC is supported and 3.3V cards only, but cards of 32GiB upwards from a number of manufacturers have been tried.
SD card test checks for the presence of a microSD card in the slot, reads and reports the card properties and then proceeds to check the contents of a test file `LOREM.IPS` in the root directory of a FAT32-formatted card against its internal reference. See the source for directions on how to set up a microSD card with a suitable file; set 'emitText' to true on the first run and it will emit the sample text with instructions. Support for Long FileNames.
SD card model implements single block write/read in the presence of a suitable 'sd.img' but if that file is not present then the invalid CID/CSD registers must be used by the CI system in simulation to suppress data block transfers.
57db43f
to
0712ec9
Compare
void select_card(bool enable) { spi->cs = enable ? (spi->cs & ~cs) : (spi->cs | cs); } | ||
|
||
// Initialise the SD card ready for use. | ||
bool init() { |
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.
In my experience working with SD cards, it's better to actually do the initialization properly because you will find cards in the while that enforce the spec to the letter.
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.
Thanks for all the feedback; the code was originally a quick prototype just to check that we could use the SD card at all before 1.0, and it was based upon rather ad hoc tutorials/code found online, without having a proper specification.
It's only example code, but perhaps - at our leisure - we should shore it up properly; @GregAC plans to improve the robustness and error detection of the code.
|
||
// Note that this is a very stripped-down card initialisation sequence | ||
// that assumes SDHC version 2, so use a more recent microSD card. | ||
do { |
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.
Per the spec, a single command should suffice here, do you really need to loop?
send_command(CMD_GO_IDLE_STATE, 0u); | ||
} while (0x01 != get_response_R1()); | ||
|
||
send_command(CMD_SEND_IF_COND, 0x1aau); |
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.
Supporting a v1 card is not actually hard: if the card does not answer this command then it's v1. Maybe at least print an error message and bail out?
// Initialise the SD card ready for use. | ||
bool init() { | ||
// Every card tried seems to be more than capable of keeping up with 20Mbps. | ||
constexpr unsigned kSpiSpeed = 0u; |
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 believe that in theory, you are not supposed to go above 400kHz when initializating the card.
|
||
// Read card capacity information. | ||
send_command(CMD_READ_OCR, 0); | ||
get_response_R3(); |
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 believe that once you get a successful answer from CMD58 you can switch to the default clock rate which is 20MHz for SPI mode.
log->println("Sending command {:#04x}", cmdCode); | ||
} | ||
|
||
// Apparently we need to clock 8 times before sending the command. |
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.
This is actually the opposite: you need 8 clock ticks after commands. Per the spec:
After the last SD Memory Card bus transaction, the host is required, to provide 8 (eight) clock
cycles for the card to complete the operation before shutting down the clock. Following is a list of the
various bus transactions:
•A command with no response. 8 clocks after the host command end bit.
•A command with response. 8 clocks after the card response end bit.
•A read data transaction. 8 clocks after the end bit of the last data block.
•A write data transaction. 8 clocks after the CRC status token.
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.
Approving this, but let's wait for CI to pass before merging.
Re the SD card image file, I'd rather not have everyone who wants to check out the sonata-system repository need to fetch a massive file. Is there any reason we need such a large file? Alternatively we could store it compressed (I'm assuming as an empty file system image it'll compress very very well) Is there a way to mark an LFS file as 'optional' or something? I.e. a normal clone won't download it and you need another command to actually fetch it. |
I remarked earlier that I don't know how small the image can be and still be usable. 1GiB is merely the size I've been using until now, but it may even work fine with as little as 8KiB, if software can create a suitable image. For now it doesn't matter, we've just restricted the testing in simulation; to be addressed later. It's still exercising the SPI and microSD card interface and doing data reads from CID and CSD in sim, just not from simulated flash storage. |
CI failures are expected; there is no microSD card( image) in
eithersimulationor FPGA testingat present.This PR does the following:
Commit 1:
Commit 2:
Whilst 'sdraw_check' may be run against any modern microSD card of suitable capacity that does not contain data you wish to keep, the test program in the second commit requires that the card be prepared with a suitable file 'LOREM.IPS' (with that exact name and capitalisation at present). Detailed instructions may be found in the source file
sw/cheri/tests/sdcard_tests.hh
; in outline, modify that file by setting 'emitText = true' before running it for the first time and it will emit the instructions and the sample text that must be used in setting up the microSD card.Create a single FAT32 partition on the card, using the 'Master Boot Record' scheme and not 'GPT.' Newly-purchased cards should already be suitable although only 32GiB and 64GiB cards have been tried to date.
Still to come, and probably required before we can merge this (to keep the FPGA and simulation consistent):
1. DPI model of a SPI-connected SD card; this allows the above test to pass in simulation too.Still on the 'TODO' list - but perhaps may be left - until a later PR are:
1. Improved filename matching within directories, including Long FileName support.2. A fresh attempt to get multiple block read/write operations working; these are presently implemented using repeated single block reads.