Merge PR #42 from remote-tracking branch 'origin/fix-sd-spi-hwconfig' into main

Signed-off-by: Matthias Blankertz <matthias@blankertz.org>

Reviewed-on: #42
Reviewed-by: Stefan Kratochwil <kratochwil-la@gmx.de>

software/modules/rp2_sd/sd.c

@@ -130,57 +130,62 @@ static void sd_dump_cid [[maybe_unused]] (void)
 static bool sd_read_csd(struct sd_context *sd_context)
 {
     uint8_t buf[16];
-    if (sd_cmd_read(9, 0, 16, buf)) {
+    if (!sd_cmd_read(9, 0, 16, buf)) {
-        const uint8_t crc = sd_crc7(15, buf);
+        printf("Failed to read CSD\n");
-        const uint8_t card_crc = buf[15] >> 1;
+        return false;
-        if (card_crc != crc) {
+    }
-            printf("CRC mismatch: Got %02hhx, expected %02hhx\n", card_crc, crc);
+    const uint8_t crc = sd_crc7(15, buf);
-            // Some cheap SD cards always report CRC=0, don't fail in that case
+    const uint8_t card_crc = buf[15] >> 1;
-            if (card_crc != 0) {
+    if (card_crc != crc) {
-                return false;
+        printf("CRC mismatch: Got %02hhx, expected %02hhx\n", card_crc, crc);
-            }
+        // Some cheap SD cards always report CRC=0, don't fail in that case
-        }
+        if (card_crc != 0) {
-        const unsigned csd_ver = buf[0] >> 6;
-        unsigned blocksize [[maybe_unused]] = 0;
-        unsigned blocks = 0;
-        unsigned version [[maybe_unused]] = 0;
-        switch (csd_ver) {
-        case 0: {
-            if (sd_context->sdhc_sdxc) {
-                printf("sd_init: Got CSD v1.0 but card is SDHC/SDXC?\n");
-                return false;
-            }
-            const unsigned read_bl_len = buf[5] & 0xf;
-            if (read_bl_len < 9 || read_bl_len > 11) {
-                printf("Invalid read_bl_len in CSD 1.0\n");
-                return false;
-            }
-            blocksize = 1 << (buf[5] & 0xf);
-            const unsigned c_size_mult = (buf[9] & 0x1) << 2 | (buf[10] & 0xc0) >> 6;
-            const unsigned c_size = (buf[6] & 0x3) << 10 | (buf[7] << 2) | (buf[8] & 0xc0) >> 6;
-            blocks = (c_size + 1) * (1 << (c_size_mult + 2));
-            version = 1;
-            break;
-        }
-        case 1: {
-            blocksize = SD_SECTOR_SIZE;
-            const unsigned c_size = (buf[7] & 0x3f) << 16 | buf[8] << 8 | buf[9];
-            blocks = (c_size + 1) * 1024;
-            version = 2;
-            break;
-        }
-        case 2: {
-            printf("sd_init: Got CSD v3.0, but SDUC does not support SPI.\n");
             return false;
         }
-        }
-        sd_context->blocks = blocks;
-        sd_context->blocksize = blocksize;
-#ifdef SD_DEBUG
-        printf("CSD version %u.0, blocksize %u, blocks %u, capacity %llu MiB\n", version, blocksize, blocks,
-               ((uint64_t)blocksize * blocks) / (1024 * 1024));
-#endif
     }
+    const unsigned csd_ver = buf[0] >> 6;
+    unsigned blocksize [[maybe_unused]] = 0;
+    unsigned blocks = 0;
+    unsigned version [[maybe_unused]] = 0;
+    unsigned max_speed [[maybe_unused]] = 0;
+    switch (csd_ver) {
+    case 0: {
+        if (sd_context->sdhc_sdxc) {
+            printf("sd_init: Got CSD v1.0 but card is SDHC/SDXC?\n");
+            return false;
+        }
+        const unsigned read_bl_len = buf[5] & 0xf;
+        if (read_bl_len < 9 || read_bl_len > 11) {
+            printf("Invalid read_bl_len in CSD 1.0\n");
+            return false;
+        }
+        blocksize = 1 << (buf[5] & 0xf);
+        const unsigned c_size_mult = (buf[9] & 0x1) << 2 | (buf[10] & 0xc0) >> 6;
+        const unsigned c_size = (buf[6] & 0x3) << 10 | (buf[7] << 2) | (buf[8] & 0xc0) >> 6;
+        blocks = (c_size + 1) * (1 << (c_size_mult + 2));
+        version = 1;
+        max_speed = buf[3];
+        break;
+    }
+    case 1: {
+        blocksize = SD_SECTOR_SIZE;
+        const unsigned c_size = (buf[7] & 0x3f) << 16 | buf[8] << 8 | buf[9];
+        blocks = (c_size + 1) * 1024;
+        version = 2;
+        max_speed = buf[3];
+        break;
+    }
+    case 2: {
+        printf("sd_init: Got CSD v3.0, but SDUC does not support SPI.\n");
+        return false;
+    }
+    }
+    sd_context->blocks = blocks;
+    sd_context->blocksize = blocksize;
+#ifdef SD_DEBUG
+    printf("CSD version %u.0, blocksize %u, blocks %u, capacity %llu MiB, max speed %u\n", version, blocksize, blocks,
+           ((uint64_t)blocksize * blocks) / (1024 * 1024), max_speed);
+#endif
     return true;
 }
 
@@ -191,52 +196,52 @@ bool sd_init(struct sd_context *sd_context, int mosi, int miso, int sck, int ss,
     }
 
     if (!sd_early_init()) {
-        return false;
+        goto out_spi;
     }
 
     if (!sd_check_interface_condition()) {
-        return false;
+        goto out_spi;
     }
 
     uint32_t ocr;
     if (!sd_read_ocr(&ocr)) {
         printf("sd_init: read OCR failed\n");
-        return false;
+        goto out_spi;
     }
     if ((ocr & 0x00380000) != 0x00380000) {
         printf("sd_init: unsupported card voltage range\n");
-        return false;
+        goto out_spi;
     }
 
     if (!sd_send_op_cond())
-        return false;
+        goto out_spi;
 
     sd_spi_set_bitrate(rate);
 
     if (!sd_read_ocr(&ocr)) {
         printf("sd_init: read OCR failed\n");
-        return false;
+        goto out_spi;
     }
     if (!(ocr & (1 << 31))) {
         printf("sd_init: card not powered up but !idle?\n");
-        return false;
+        goto out_spi;
     }
     sd_context->sdhc_sdxc = (ocr & (1 << 30));
 
     if (!sd_read_csd(sd_context)) {
-        return false;
+        goto out_spi;
     }
 
     if (sd_context->blocksize != SD_SECTOR_SIZE) {
         if (sd_context->blocksize != 1024 && sd_context->blocksize != 2048) {
             printf("sd_init: Unsupported block size %u\n", sd_context->blocksize);
-            return false;
+            goto out_spi;
         }
         // Attempt SET_BLOCKLEN command
         uint8_t resp[1];
         if (!sd_cmd(16, SD_SECTOR_SIZE, 1, resp)) {
             printf("sd_init: SET_BLOCKLEN failed\n");
-            return false;
+            goto out_spi;
         }
         // Successfully set blocksize to SD_SECTOR_SIZE, adjust context
         sd_context->blocks *= sd_context->blocksize / SD_SECTOR_SIZE;
@@ -253,6 +258,10 @@ bool sd_init(struct sd_context *sd_context, int mosi, int miso, int sck, int ss,
 
     sd_context->initialized = true;
     return true;
+
+out_spi:
+    sd_spi_deinit();
+    return false;
 }
 
 bool sd_deinit(struct sd_context *sd_context)

software/modules/rp2_sd/sd_spi.c

@@ -220,6 +220,12 @@ bool sd_cmd_read_complete(void)
     sd_spi_wait_complete();
     gpio_put(sd_spi_context.ss, true);
     sd_spi_read_blocking(0xff, &buf, 1);
+    if (sd_spi_context.sd_dma_context.read_token_buf != 0xfe) {
+#ifdef SD_DEBUG
+        printf("read failed: invalid read token %02hhx\n", sd_spi_context.sd_dma_context.read_token_buf);
+#endif
+        return false;
+    }
 #ifdef SD_READ_CRC_CHECK
     const uint16_t expect_crc = sd_crc16(sd_spi_context.sd_dma_context.len, sd_spi_context.sd_dma_context.read_buf);
     const uint16_t act_crc = sd_spi_context.sd_dma_context.crc_buf[0] << 8 | sd_spi_context.sd_dma_context.crc_buf[1];
@@ -230,7 +236,7 @@ bool sd_cmd_read_complete(void)
         return false;
     }
 #endif
-    return (sd_spi_context.sd_dma_context.read_token_buf == 0xfe);
+    return true;
 }
 
 bool sd_cmd_write(uint8_t cmd, uint32_t arg, unsigned datalen, uint8_t data[const static datalen])

software/modules/rp2_sd/sd_spi_pio.pio

@@ -52,10 +52,12 @@ static inline void sd_spi_pio_program_init(PIO pio, uint sm, uint offset, uint m
     sm_config_set_out_shift(&c, false, true, 8);
     sm_config_set_in_shift(&c, false, true, 8);
 
+    // high speed SPI needs to bypass the input synchronizers on the MISO pin
+    hw_set_bits(&pio->input_sync_bypass, 1u << miso);
+
     const unsigned pio_freq = bitrate*4;
     const float div = clock_get_hz(clk_sys) / (float)pio_freq;
-    // for some reason, small clkdiv values (even integer ones) cause issues
+    sm_config_set_clkdiv(&c, div);
-    sm_config_set_clkdiv(&c, div < 2.5f ? 2.5f : div);
     pio_sm_init(pio, sm, offset, &c);
 }
 %}

software/src/hwconfig_Rev1.py

@@ -9,6 +9,7 @@ SD_DI = Pin.board.GP3
 SD_DO = Pin.board.GP4
 SD_SCK = Pin.board.GP2
 SD_CS = Pin.board.GP5
+SD_CLOCKRATE = 25000000
 
 # MAX98357
 I2S_LRCLK = Pin.board.GP6
@@ -44,10 +45,11 @@ def board_init():
     # POWER_EN turned on in MICROPY_BOARD_STARTUP
     # TODO: Implement soft power off
 
-    # Set 8 mA drive strength and fast slew rate for SD SPI
+    # SD_DO / MISO input doesn't need any special configuration
-    machine.mem32[0x4001c004 + 6*4] = 0x67
+    # Set 8 mA drive strength for SCK and MOSI
-    machine.mem32[0x4001c004 + 7*4] = 0x67
+    machine.mem32[0x4001c004 + 2*4] = 0x60  # SCK
-    machine.mem32[0x4001c004 + 8*4] = 0x67
+    machine.mem32[0x4001c004 + 3*4] = 0x60  # MOSI
+    # SD_CS doesn't need any special configuration
 
     # Permanently enable amplifier
     # TODO: Implement amplifier power management

software/src/hwconfig_breadboard.py

@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: MIT
 # Copyright (c) 2025 Matthias Blankertz <matthias@blankertz.org>
 
-import machine
 from machine import Pin
 
 # SD Card SPI
@@ -9,6 +8,7 @@ SD_DI = Pin.board.GP3
 SD_DO = Pin.board.GP4
 SD_SCK = Pin.board.GP2
 SD_CS = Pin.board.GP5
+SD_CLOCKRATE = 15000000
 
 # MAX98357
 I2S_LRCLK = Pin.board.GP7
@@ -41,10 +41,7 @@ VBAT_ADC = Pin.board.GP26
 
 
 def board_init():
-    # Set 8 mA drive strength and fast slew rate for SD SPI
+    pass
-    machine.mem32[0x4001c004 + 6*4] = 0x67
-    machine.mem32[0x4001c004 + 7*4] = 0x67
-    machine.mem32[0x4001c004 + 8*4] = 0x67
 
 
 def get_battery_voltage():

software/src/main.py

@@ -62,7 +62,7 @@ def run():
 
     # Setup MP3 player
     with SDContext(mosi=hwconfig.SD_DI, miso=hwconfig.SD_DO, sck=hwconfig.SD_SCK, ss=hwconfig.SD_CS,
-                   baudrate=15000000), \
+                   baudrate=hwconfig.SD_CLOCKRATE), \
          BTreeFileManager('/sd/tonberry.db') as playlistdb, \
          AudioContext(hwconfig.I2S_DIN, hwconfig.I2S_DCLK, hwconfig.I2S_LRCLK) as audioctx:
 

software/tools/standalone-mp3/CMakeLists.txt

@@ -10,6 +10,7 @@ include(../../lib/micropython/lib/pico-sdk/pico_sdk_init.cmake)
 project(standalone_mp3)
 
 option(ENABLE_WRITE_TEST "Enable write test" OFF)
+option(ENABLE_READ_TEST "Enable read test" ON)
 option(ENABLE_PLAY_TEST "Enable mp3 playback test" OFF)
 option(ENABLE_SD_READ_CRC "Enable crc check when reading from sd card" OFF)
 option(ENABLE_SD_DEBUG "Enable debug output for sd card driver" OFF)
@@ -37,6 +38,10 @@ if(ENABLE_WRITE_TEST)
     target_compile_definitions(standalone_mp3 PRIVATE WRITE_TEST)
 endif()
 
+if(ENABLE_READ_TEST)
+    target_compile_definitions(standalone_mp3 PRIVATE READ_TEST)
+endif()
+
 if(ENABLE_PLAY_TEST)
     target_compile_definitions(standalone_mp3 PRIVATE PLAY_TEST)
 endif()

software/tools/standalone-mp3/main.c

@@ -157,7 +157,7 @@ static void write_test(struct sd_context *sd_context)
             data_buffer[i] ^= 0xff;
         }
 
-        if(!sd_writeblock(sd_context, 0, data_buffer)) {
+        if (!sd_writeblock(sd_context, 0, data_buffer)) {
             printf("sd_writeblock failed\n");
             return;
         }
@@ -165,6 +165,20 @@ static void write_test(struct sd_context *sd_context)
     } while (data_buffer[SD_SECTOR_SIZE - 1] != 0xAA);
 }
 
+static void read_test(struct sd_context *sd_context)
+{
+    uint8_t data_buffer[512];
+    const uint64_t before = time_us_64();
+    for (int block = 0; block < 245760; ++block) {
+        if (!sd_readblock(sd_context, block, data_buffer)) {
+            printf("sd_readblock(%d) failed\n", block);
+            return;
+        }
+    }
+    const uint64_t elapsed = time_us_64() - before;
+    printf("%llu ms elapsed, %f kB/s\n", elapsed / 1000LLU, 128 * 1024.f / (elapsed / 1000000.f));
+}
+
 int main()
 {
     stdio_init_all();
@@ -172,10 +186,23 @@ int main()
 
     struct sd_context sd_context;
 
-    if (!sd_init(&sd_context, 3, 4, 2, 5, 15000000)) {
+#define DRIVE_STRENGTH GPIO_DRIVE_STRENGTH_8MA
+#define SLEW_RATE GPIO_SLEW_RATE_SLOW
+
+    gpio_set_drive_strength(2, DRIVE_STRENGTH);
+    gpio_set_slew_rate(2, SLEW_RATE);
+    gpio_set_drive_strength(3, DRIVE_STRENGTH);
+    gpio_set_slew_rate(3, SLEW_RATE);
+
+    if (!sd_init(&sd_context, 3, 4, 2, 5, 25000000)) {
+        printf("sd_init failed\n");
         return 1;
     }
 
+#ifdef READ_TEST
+    read_test(&sd_context);
+#endif
+
 #ifdef WRITE_TEST
     write_test(&sd_context);
 #endif