[spi_host] Low-level SD card support by elliotb-lowrisc · Pull Request #446 · lowRISC/mocha

elliotb-lowrisc · 2026-04-17T15:17:41Z

Replace the initial SPI Host loopback connections with connections to the microSD card slot for Genesys2 and an SD card model for Verilator. This includes allocating a GPI pin for card presence detection and a GPO pin for SD card reset (power supply switching).

Also output the SPI Host signals on an otherwise unused Genesys2 PMOD header for easier debugging using a logic analyser.

Add SD card and minimal filesystem helpers for SPI Host, and some tests. These have been ported from sonata-system, which required changes for the different spi_host hardware and translating from C++ to C.

Only a subset of the tests will run in Verilator unless an SD card image is provided.

elliotb-lowrisc · 2026-05-08T14:26:46Z

Reduced the level of UART logging when logging = false to avoid timing-out in the Verilator test suite

engdoreis · 2026-05-08T14:45:25Z

+## SPI Host (MicroSD card slot)
+set_property -dict { PACKAGE_PIN R28   IOSTANDARD LVCMOS33 } [get_ports { spi_host_sck_o }]; # SD_SCLK
+set_property -dict { PACKAGE_PIN R26   IOSTANDARD LVCMOS33 } [get_ports { spi_host_sd_i  }]; # SD_DAT0
+# set_property -dict { PACKAGE_PIN R30   IOSTANDARD LVCMOS33 } [get_ports { microsd_dat1 }]; # SD_DAT1 unused in SPI bus mode


Any reason to keep the commented line?

It keeps a record of how to connect up the other SD card pins, should they be needed in future.

I would prefer not having commented lines as it polutes the file, the history record can always be recovered with git.

engdoreis · 2026-05-08T15:08:42Z

+void uart_dump_bytes(uart_t uart, const uint8_t buf[], size_t len)
+{
+    for (size_t off = 0u; off < len; ++off) {
+        uart_putchar(uart, '0' + (buf[off] >> 4));


Please use the printf functions to implement the dump:
https://github.com/lowRISC/mocha/blob/main/sw/device/lib/runtime/print.h

Wouldn't that create a circular reference?

I seem to have more capacity for problem solving today. I can move this function to a file of its own to get around the dependency issue, and buffer bytes into four-byte bundles to work around the lack of width/padding control in uprintf at present.

The result looks like this:

cd16cde4 54feeb19 20736968 6e207369 : .......This is n 6120746f 6f6f6220 6c626174 69642065 : ot a bootable di 202e6b73 656c5020 20657361 65736e69 : sk. Please inse

Which seems good enough for now, if perhaps not as readable as the original intent:

E4 CD 16 CD 19 EB FE 54 68 69 73 20 69 73 20 6E : .......This is n 6F 74 20 61 20 62 6F 6F 74 61 62 6C 65 20 64 69 : ot a bootable di 73 6B 2E 20 20 50 6C 65 61 73 65 20 69 6E 73 65 : sk. Please inse

engdoreis · 2026-05-08T15:09:19Z

+#include "runtime/filesys_utils.h"
+#include "runtime/print.h"
+#include "runtime/sdcard.h"
+// #include <assert.h>


drop commented line.

Are we not going to be adding support for asserts in the future?

engdoreis · 2026-05-08T15:10:54Z

+
+
+// Copy a sequence of bytes; destination and source must _not_ overlap.
+static void copy_bytes(uint8_t *dst, const uint8_t *src, size_t len)


use memcpy from: https://github.com/lowRISC/mocha/blob/main/sw/device/lib/runtime/string.h

Thanks, I'll give this a go next time I have access to an FPGA

Hmm, not sure then that'll be actually, so I'll try it in CI

engdoreis · 2026-05-08T15:13:49Z

+// public:
+// Unfortunately FAT stores the literal values for bytes/sector and sectors/cluster but only
+// powers of two are permitted.
+static inline uint8_t floor_log2(uint16_t n)


I think you can use a builtin_ctz here:

mocha/sw/device/lib/builtin.h

Line 14 in 5a99b1d

#define ctz(x) (__builtin_ctz((x)))

I think we want to find the leading set bit, not the trailing set bit

I believe floor log2 (sometimes called ilog2) would be <bitwidth of x> - clz(x) - 1, with an exception for x = 0. Those would be useful to have in builtin.h.

Something like this?

#define ilog2_16b(x) (x == 0 ? 0 : (16 - clz(x) - 1))

Nope, that didn't work. I think I'm missing how to get the bitwidth of x as far as clz is concerned

engdoreis · 2026-05-08T15:15:38Z

+    uint8_t *dataBuffer = fs->buf.dataBuffer;
+    if (!read_blocks(fs->spi, 0, dataBuffer, 1u, fs->uart)) {
+        if (fs->uart) {
+            uart_puts(fs->uart, "Unable to read the MBR of the SD card\n");


I would use uprintf everywhere for consistency

Why use a formatting printer just to output a string?

If you'd both prefer it, I suppose I can switch to uprintf everywhere and put all non-error messages behind logging gating to avoid making what will be the slowest Verilator test even slower (roughly 10% slower from the switch before I added the extra gating)

For a simple string, the uprintf should perform almos the same, since there won't be args parse and format.
But I don't have strong opinion.

engdoreis · 2026-05-08T15:24:59Z

+}
+
+// Open a directory object that started in the given cluster.
+fs_utils_dir_handle_t dir_open(fs_utils_state_t *fs, uint32_t cluster)


This is a lot of code to maintain, for the purpose of testing the sdcard, handling directories is not important. So I suggest droping the directory handling and keeping the bare minimum flat file handling.
Specially because the real use case of the SDCard will be done in Uboot which already has a filesystem implementation.

I'm confused. This code is used in the SD Card test I added. Are you suggesting performing a lesser test?

In any case, I'd have thought it useful to keep this code in case we want to to use bare-metal SD card access in future, such as for a demo.

I believe that using directories in the filesystem would not increase any test converage of the hardware. As a reference in Opentitan we don't need to mount a filesystem to test that spi_host works properly with the flashes.
I'm just concern that with more code in the repo, the harder to debug tests the future and therefore I advocate for the bare minimum.

But I'm happy to keep it if others desagree.

engdoreis · 2026-05-08T15:28:54Z

+{
+    // Every card tried seems to be more than capable of keeping up with 12.5Mbps.
+    const unsigned kSpiSpeed = 1u; // 50 MHz / (1 + 1) = 12.5 MHz
+    DEV_WRITE(spi + SPI_HOST_CONFIGOPTS_REG, 0xFFFF & kSpiSpeed);


Why not use the spi_host driver instead of directly accessing the mmios?

The spi_host HAL does not presently support many of the features of the spi_host block that this code needs to make the SD card communications work. Perhaps the sdcard code could be re-written if the HAL is sufficiently improved, but until then I thought it better to just stick to using the spi_host block directly rather than only partially using the HAL.

I would than advocate to improve the spi_host HAL, to avoid duplicate code when this features are needed there.

marnovandermaas

Thanks for working on this! I've left some initial comments.

marnovandermaas · 2026-05-08T15:39:06Z

-# Bootstrap pin, should be pulled down during boot to enter bootstrap mode.
-set_property -dict { PACKAGE_PIN AB29  IOSTANDARD LVCMOS33 PULLTYPE PULLUP } [get_ports { gpio_i[8] }];
+#   Bootstrap pin, should be pulled down during boot to enter bootstrap mode.
+set_property -dict { PACKAGE_PIN AB29  IOSTANDARD LVCMOS33 PULLTYPE PULLUP } [get_ports { gpio_i[8] }]; # PROG_RXFN


What does "PROG_RXFN" mean?

It's the name in the Genesys2 schematic for the net connected to this pin. Unlike Sonata, we're not using the schematic names for the FPGA-top signal names (see #411), so I'm adding them as a comment instead.

marnovandermaas · 2026-05-08T15:42:25Z

    .rst_ni,

-    .gpio_i    (gpio_inputs),
+    .gpio_i    ({gpio_inputs[31:10], microsd_det, gpio_inputs[8:0]}),


This is a little bit ugly because the gpio_input now will just ignore any writes to bit index 9. I'm not sure how to solve it more cleanly though.

Indeed, I'm open to better ideas

marnovandermaas · 2026-05-08T15:59:20Z

  logic       spi_device_sdi;

+  // SPI host signals
+  logic       spi_host_sck_output,    spi_host_csb_output;


I'm not sure I think the "output" tag is more clear here. If you don't like just calling the signals spi_host_* then maybe we can label it h2d for host to device or something similar to indicate direction.

Ah right, I think I was following something like what I did for GPIO, but this is not bidirectional so things can be simpler

Better now?

elliotb-lowrisc · 2026-05-13T15:49:19Z

Use memcpy, shorten some Verilator signal names, doc fix

elliotb-lowrisc · 2026-05-14T15:28:50Z

Rework uart printing to use uprintf everywhere as requested

Replace the initial SPI Host loopback connections with connections to the microSD card slot for Genesys2 and an SD card model for Verilator. This includes allocating a GPI pin for card presence detection and a GPO pin for SD card reset (power supply switching). Also output the SPI Host signals on an otherwise unused Genesys2 PMOD header for easier debugging using a logic analyser.

elliotb-lowrisc · 2026-05-15T08:16:16Z

Rebase

Add SD card and minimal filesystem helpers for SPI Host, and some tests. These have been ported from sonata-system, which required changes for the different spi_host hardware and translating from C++ to C. Only a subset of the tests will run in Verilator unless an SD card image is provided.

elliotb-lowrisc · 2026-05-15T10:10:14Z

Lint fixes (I forgot to run the formatter after the last round of changes)

elliotb-lowrisc mentioned this pull request Apr 17, 2026

[hw] SPI host SD card integration #402

Open

elliotb-lowrisc linked an issue Apr 24, 2026 that may be closed by this pull request

Draft hardware changes for SPI - SD card integration #468

Closed

elliotb-lowrisc removed a link to an issue Apr 24, 2026

Draft hardware changes for SPI - SD card integration #468

Closed

elliotb-lowrisc mentioned this pull request Apr 24, 2026

Draft hardware changes for SPI - SD card integration #468

Closed

elliotb-lowrisc linked an issue Apr 24, 2026 that may be closed by this pull request

[hw] SPI host SD card integration #402

Open

elliotb-lowrisc force-pushed the spi_sd branch 2 times, most recently from 08ccd27 to db2ad39 Compare May 7, 2026 15:44

elliotb-lowrisc changed the title ~~[spi_host] Switch from loopback to SD card access~~ [spi_host] Low-level SD card support May 7, 2026

elliotb-lowrisc mentioned this pull request May 7, 2026

Merge SD card HW/SW changes to Mocha #534

Open

elliotb-lowrisc linked an issue May 7, 2026 that may be closed by this pull request

Merge SD card HW/SW changes to Mocha #534

Open

engdoreis reviewed May 8, 2026

View reviewed changes

Comment thread sw/device/lib/runtime/filesys_utils.h Outdated

elliotb-lowrisc force-pushed the spi_sd branch 3 times, most recently from c1e2865 to 0510cd1 Compare May 8, 2026 14:25

elliotb-lowrisc marked this pull request as ready for review May 8, 2026 15:13

engdoreis reviewed May 8, 2026

View reviewed changes

marnovandermaas reviewed May 8, 2026

View reviewed changes

elliotb-lowrisc force-pushed the spi_sd branch from 0510cd1 to 5ded2aa Compare May 13, 2026 15:38

elliotb-lowrisc force-pushed the spi_sd branch from 5ded2aa to 9dbc8b2 Compare May 14, 2026 15:28

elliotb-lowrisc force-pushed the spi_sd branch from 9dbc8b2 to e4072db Compare May 15, 2026 08:16

elliotb-lowrisc force-pushed the spi_sd branch from e4072db to b78bd7b Compare May 15, 2026 10:09



		// Copy a sequence of bytes; destination and source must _not_ overlap.
		static void copy_bytes(uint8_t dst, const uint8_t src, size_t len)

Conversation

elliotb-lowrisc commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elliotb-lowrisc commented May 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marnovandermaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elliotb-lowrisc commented May 13, 2026

Uh oh!

elliotb-lowrisc commented May 14, 2026

elliotb-lowrisc commented Apr 17, 2026 •

edited

Loading