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

SD card access and Read Only FAT32 file system support #360

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

alees24
Copy link
Contributor

@alees24 alees24 commented Nov 28, 2024

CI failures are expected; there is no microSD card( image) in either simulationor FPGA testing at present.

This PR does the following:

Commit 1:

  • Introduces SD card support on top of the SPI driver
  • Adds a manual 'check' program called 'sdraw_check' that insists upon manual insertion of a card that may be corrupted by the test, and then performs a number of randomised block writes and reads to check that these occur successfully.

Commit 2:

  • Introduces Read Only support for a single FAT32 partition (as SD cards are typically shipped by manufacturers).
  • Adds SD card testing to the 'test_runner' regression test in CI to test (i) SD card presence, (ii) the ability to read the card properties, (iii) the ability to read a sample text file from the root directory of a FAT32 partition on the SD card (iv) that it matches against a reference copy.

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.

Copy link
Contributor

@marnovandermaas marnovandermaas left a 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.

sw/cheri/checks/sdraw_check.cc Show resolved Hide resolved
sw/cheri/checks/sdraw_check.cc Show resolved Hide resolved
sw/cheri/checks/sdraw_check.cc Outdated Show resolved Hide resolved
sw/cheri/checks/sdraw_check.cc Outdated Show resolved Hide resolved
sw/cheri/common/sdcard-utils.hh Outdated Show resolved Hide resolved
sw/cheri/tests/sdcard_tests.hh Outdated Show resolved Hide resolved
sw/cheri/tests/sdcard_tests.hh Outdated Show resolved Hide resolved
sw/cheri/checks/sdraw_check.cc Show resolved Hide resolved
@GregAC
Copy link
Contributor

GregAC commented Nov 29, 2024

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 sdcard-utils.hh live? We could hopefully refactor things so it can work both in the bare metal environment and CHERIoT RTOS environment. I think it's best left in this repository so we don't introduce a new sonata-system -> sonata-software dependency. We have a sonata-software -> sonata-system link though it's done via nix. In order to provide easy code access for people who aren't using the nix environment (i.e. we want a way to build an SD card example in sonata-software without relying on some nix magic to pull the sdcard-utils.hh into the right place) either we bring sonata-system in as a sub-module or we just make a copy of it/vendor it in sonata-software. I think I favour the latter as it's just one file and we don't really want the whole of sonata-system as a big submodule.

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.

@alees24
Copy link
Contributor Author

alees24 commented Nov 29, 2024

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.

@GregAC
Copy link
Contributor

GregAC commented Nov 29, 2024

The DPI model already exists and works;

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.

@alees24 alees24 marked this pull request as ready for review November 29, 2024 23:30
@alees24
Copy link
Contributor Author

alees24 commented Nov 29, 2024

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!

@alees24 alees24 force-pushed the sdcard-access branch 2 times, most recently from 5ece601 to db37f73 Compare November 30, 2024 00:26
@alees24
Copy link
Contributor Author

alees24 commented Dec 5, 2024

I see a few possible options for resolving the simulator situation:

  • put the file 'sd.img' in the repository; some people don't like binary files in repositories, and it may need to be quite large; I don't presently know the minimum capacity volume that the code will accept.
  • the SD DPI model could be modified to construct a suitable image if there is not one available, or this could be done as a step in the CI build flow; a short sequence of shell commands ought to suffice, and the reference text would need to be extracted from the header file and written as a file on the mounted virtual volume.
  • sdcard_tests.hh operates differently on sim c.f. FPGA and does not access a disk image; I'd prefer to avoid this disparity if possible.

Opinions/thoughts, please?

@marnovandermaas
Copy link
Contributor

I see a few possible options for resolving the simulator situation:

* put the file 'sd.img' in the repository; some people don't like binary files in repositories, and it may need to be quite large; I don't presently know the minimum capacity volume that the code will accept.

If we need to go done this route, we can use git large file storage.

* the SD DPI model could be modified to construct a suitable image if there is not one available, or this could be done as a step in the CI build flow; a short sequence of shell commands ought to suffice, and the reference text would need to be extracted from the header file and written as a file on the mounted virtual volume.

My initial though is that this is best. Would you be able to have a go at this?

* sdcard_tests.hh operates differently on sim c.f. FPGA and does not access a disk image; I'd prefer to avoid this disparity if possible.

Opinions/thoughts, please?

See my opinions inline.

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.
void select_card(bool enable) { spi->cs = enable ? (spi->cs & ~cs) : (spi->cs | cs); }

// Initialise the SD card ready for use.
bool init() {
Copy link

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.

Copy link
Contributor Author

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 {
Copy link

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);
Copy link

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;
Copy link

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();
Copy link

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.
Copy link

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.

Copy link
Contributor

@marnovandermaas marnovandermaas left a 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.

@GregAC
Copy link
Contributor

GregAC commented Dec 9, 2024

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.

@alees24
Copy link
Contributor Author

alees24 commented Dec 9, 2024

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.

@alees24 alees24 merged commit 181cb38 into lowRISC:main Dec 9, 2024
3 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.

5 participants