From 38ec1e1bed84ca5a9983056e0f29e1e4cb0a4efc Mon Sep 17 00:00:00 2001 From: David Garske Date: Tue, 11 Aug 2026 13:17:43 -0700 Subject: [PATCH] sdhci: fix silent read failures and unbootable warm reset with UHS-I cards --- .gitignore | 1 + include/sdhci.h | 3 + src/sdhci.c | 83 ++++++++++- src/update_disk.c | 12 +- tools/unit-tests/Makefile | 6 +- tools/unit-tests/unit-sdhci-dma-error.c | 183 ++++++++++++++++++++++++ 6 files changed, 278 insertions(+), 10 deletions(-) create mode 100644 tools/unit-tests/unit-sdhci-dma-error.c diff --git a/.gitignore b/.gitignore index 6358c2a6b8..86117dfd60 100644 --- a/.gitignore +++ b/.gitignore @@ -191,6 +191,7 @@ tools/unit-tests/unit-update-flash-hwswap tools/unit-tests/unit-uart-flash tools/unit-tests/unit-max-space tools/unit-tests/unit-sdhci-disk-unaligned +tools/unit-tests/unit-sdhci-dma-error tools/unit-tests/unit-fwtpm-stub tools/unit-tests/unit-gzip tools/unit-tests/unit-fit-gzip diff --git a/include/sdhci.h b/include/sdhci.h index 21e89ea40c..9a8bc095b1 100644 --- a/include/sdhci.h +++ b/include/sdhci.h @@ -313,6 +313,9 @@ #define SDHCI_SRS15_HV4E (1U << 28) /* Host version 4 enable */ #define SDHCI_SRS15_UMS_MASK (0x7U << 16) #define SDHCI_SRS15_UMS_SDR25 (0x1U << 16) +/* Host Control 2 occupies the upper 16 bits of SRS15, so 1.8V Signaling + * Enable (bit 3 of Host Control 2) lands at bit 19. */ +#define SDHCI_SRS15_V18SE (1U << 19) /* 1.8V signaling enable */ #define SDHCI_SRS15_DSS_MASK (0x3U << 20) #define SDHCI_SRS15_DSS_TYPE_B (0x0U << 20) #define SDHCI_SRS15_EXTNG (1U << 22) /* Execute tuning */ diff --git a/src/sdhci.c b/src/sdhci.c index 60ae4d32cb..6d049bd60a 100644 --- a/src/sdhci.c +++ b/src/sdhci.c @@ -76,6 +76,11 @@ static uint32_t g_sector_count; static uint32_t g_sector_size; static uint32_t g_bus_width = 1; static uint32_t g_rca = 0; /* SD Card Relative Address */ +#ifdef DISK_SDCARD +/* Set once sdhci_uhs_recover() has switched the host to 1.8V signaling. + * SD-only: UHS-I signaling does not apply to eMMC. */ +static int g_uhs_recovered = 0; +#endif /* DISK_SDCARD */ /* MMC Interrupt state - volatile for interrupt handler access */ static volatile uint32_t g_mmc_irq_status = 0; @@ -369,6 +374,53 @@ static int sdhci_set_power(uint32_t voltage) return 0; } +#ifdef DISK_SDCARD +/* Recover a card that a previous stage left in UHS-I 1.8V signaling. + * + * A card that negotiated UHS-I only returns to 3.3V when VDD is removed -- + * CMD0 does not do it, and on boards where the card supply is a fixed rail + * (ZCU102 among them) software cannot remove it at all. After a warm reset + * following an OS that used UHS, the card is therefore still at 1.8V while + * the host has come up at 3.3V. The command path tolerates the mismatch, so + * initialization and isolated reads appear to work, but sustained data + * transfers corrupt: Data CRC Error (SRS12 error bit 5) on PIO, and on SDMA + * a transfer that never completes. + * + * The condition cannot be detected up front. The warm reset clears the + * controller registers, so the inherited 1.8V Signaling Enable bit is gone + * before the bootloader runs, and the corruption is marginal enough that a + * short probe read usually succeeds. So this is driven from an actual data + * failure: switch the host to meet the card, then let the caller retry. + * + * This restores signaling the card is already using rather than initiating a + * voltage switch, so no CMD11 sequence is involved. It runs at most once per + * boot, and only after a transfer has already failed, so a cold boot never + * reaches it. + * + * Returns 0 if the switch was applied and the caller should retry. */ +static int sdhci_uhs_recover(void) +{ + if (g_uhs_recovered) { + return -1; /* already tried; the failure is something else */ + } + g_uhs_recovered = 1; + + wolfBoot_printf("SDHCI: data transfer failed at 3.3V; card appears to be " + "left in UHS-I by a previous stage, retrying at 1.8V\n"); + + /* Stop the SD clock while the signaling level changes */ + sdhci_reg_and(SDHCI_SRS11, ~SDHCI_SRS11_SDCE); + + sdhci_reg_or(SDHCI_SRS15, SDHCI_SRS15_V18SE); + udelay(5000); /* let the level shifter settle */ + + sdhci_reg_or(SDHCI_SRS11, SDHCI_SRS11_SDCE); + udelay(1000); + + return 0; +} +#endif /* DISK_SDCARD */ + /* ============================================================================ * Clock Control * ============================================================================ */ @@ -1435,28 +1487,42 @@ static int sdhci_transfer(int dir, uint32_t cmd_index, uint32_t block_addr, } } - /* Check for errors */ + /* Check for errors. + * + * An earlier failure (e.g. the SDMA wait above timing out) must survive + * this block. A wolfBoot-side wait timeout does not necessarily set an + * SRS12 error bit, so without preserving `status` the CMD12 / wait-busy + * results below would overwrite it and sdhci_transfer() would report + * success for a transfer that never completed. The caller then uses a + * partially filled buffer, which surfaces much later as a bogus image + * integrity failure rather than as the I/O error it actually is. */ reg = SDHCI_REG(SDHCI_SRS12); if ((reg & SDHCI_SRS12_ERR_STAT) == 0) { - /* If multi-block, send CMD12 to stop transfer */ + /* If multi-block, send CMD12 to stop transfer. This is issued even + * when `status` is already an error, to leave the bus in a sane + * state, but its result must not clear that error. */ if (is_multi_block) { #ifdef DISK_EMMC uint32_t stop_arg = 0; #else uint32_t stop_arg = (g_rca << SD_RCA_SHIFT); #endif + int stop_status; SDHCI_REG_SET(SDHCI_SRS12, SDHCI_SRS12_TC); /* Clear transfer complete */ /* Send stop multi-block transfer */ - status = sdhci_send_cmd_internal(SDHCI_SRS03_CMD_ABORT, + stop_status = sdhci_send_cmd_internal(SDHCI_SRS03_CMD_ABORT, MMC_CMD12_STOP_TRANS, stop_arg, SDHCI_RESP_R1B); /* Card may be busy programming data after CMD12 */ - if (status == DEVICE_BUSY) { - status = sdhci_wait_busy(1); + if (stop_status == DEVICE_BUSY) { + stop_status = sdhci_wait_busy(1); } - if (status != 0) { + if (stop_status != 0) { wolfBoot_printf("sdhci_transfer: CMD12 error\n"); + if (status == 0) { + status = stop_status; + } } } if (status == 0) { @@ -1777,6 +1843,11 @@ int disk_read(int drv, uint64_t start, uint32_t count, uint8_t *buf) block_addr, (uint32_t*)buf, read_sz); #endif } +#ifdef DISK_SDCARD + if (status != 0 && sdhci_uhs_recover() == 0) { + continue; /* retry this chunk with matched signaling */ + } +#endif /* DISK_SDCARD */ if (status != 0) { break; } diff --git a/src/update_disk.c b/src/update_disk.c index d30a37f770..a6728a8b57 100644 --- a/src/update_disk.c +++ b/src/update_disk.c @@ -496,9 +496,15 @@ void RAMFUNCTION wolfBoot_start(void) load_off += ret; } while (load_off < os_image.fw_size); - if (ret < 0) { - wolfBoot_printf("Error reading image from disk: p%d\r\n", - cur_part); + /* A short read must fail here, as an I/O error. `ret == 0` breaks the + * loop above without being negative, and a truncated load would + * otherwise sail through to the integrity check and be reported as a + * corrupt image -- pointing the operator at the wrong problem, and on + * a system with anti-rollback leaving no bootable slot at all. */ + if (ret <= 0 || load_off != os_image.fw_size) { + wolfBoot_printf("Error reading image from disk: p%d " + "(%u of %u bytes)\r\n", cur_part, + (unsigned int)load_off, (unsigned int)os_image.fw_size); selected ^= 1; continue; } diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index c07bc4ca9d..e4f4132b80 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -65,7 +65,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-update-disk unit-update-disk-oob unit-multiboot unit-boot-x86-fsp unit-loader-tpm-init unit-qspi-flash unit-fwtpm-stub unit-tpm-rsa-exp \ unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-store-sbrk \ unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \ - unit-sdhci-disk-unaligned unit-sign-encrypted-output \ + unit-sdhci-disk-unaligned unit-sdhci-dma-error unit-sign-encrypted-output \ unit-sign-hybrid-keyload \ unit-sign-header-failure \ unit-keygen-xmss-params @@ -477,6 +477,10 @@ unit-sdhci-disk-unaligned: ../../include/target.h unit-sdhci-disk-unaligned.c gcc -o $@ $^ $(CFLAGS) -ffunction-sections -fdata-sections $(LDFLAGS) \ -Wl,--gc-sections +unit-sdhci-dma-error: ../../include/target.h unit-sdhci-dma-error.c + gcc -o $@ $^ $(CFLAGS) -ffunction-sections -fdata-sections $(LDFLAGS) \ + -Wl,--gc-sections + unit-aes128: ../../include/target.h unit-extflash.c gcc -o $@ $^ $(CFLAGS) $(LDFLAGS) diff --git a/tools/unit-tests/unit-sdhci-dma-error.c b/tools/unit-tests/unit-sdhci-dma-error.c new file mode 100644 index 0000000000..d797a4064c --- /dev/null +++ b/tools/unit-tests/unit-sdhci-dma-error.c @@ -0,0 +1,183 @@ +/* unit-sdhci-dma-error.c + * + * Regression test: a failed SDMA data transfer must be reported as an error. + * + * sdhci_transfer() waits for Transfer Complete on the SDMA path and sets + * status = -1 if that wait times out. The post-transfer "check for errors" + * block then runs unconditionally, and a wolfBoot-side wait timeout does not + * necessarily leave an error bit set in SRS12. Before the fix, the CMD12 + * (stop transmission) result overwrote that -1, so the function returned + * success for a transfer that never moved any data. + * + * The caller then treats a partially filled buffer as a good read. On a + * verified-boot target that surfaces much later as an image integrity + * failure rather than as the I/O error it actually is -- and where + * anti-rollback is enabled, the good lower-versioned slot is refused too, + * leaving nothing bootable. + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#define DISK_SDCARD 1 + +#include +#include +#include + +#include "sdhci.h" + +static uint32_t mock_regs[0x260 / sizeof(uint32_t)]; + +/* When 0, a data transfer never signals Transfer Complete -- the SDMA wait + * times out, which is the condition under test. When 1, the transfer + * completes normally (positive control). */ +static int mock_complete_transfer; + +/* Monotonic, so udelay() in the included sdhci.c always terminates. A + * constant timer makes its "while (hal_get_timer_us() < end)" loop spin + * forever if any code path under test ever calls it. */ +uint64_t hal_get_timer_us(void) +{ + static uint64_t now; + return ++now; +} + +uint32_t sdhci_reg_read(uint32_t offset) +{ + return mock_regs[offset / sizeof(uint32_t)]; +} + +void sdhci_reg_write(uint32_t offset, uint32_t val) +{ + uint32_t *reg = &mock_regs[offset / sizeof(uint32_t)]; + + /* Reset bits self-clear, and the internal clock reports stable */ + if (offset == SDHCI_SRS11) { + *reg = val | (val & SDHCI_SRS11_ICE ? SDHCI_SRS11_ICS : 0); + *reg &= ~SDHCI_SRS11_RESET_DAT_CMD; + return; + } + + /* SRS12 is write-1-to-clear */ + if (offset == SDHCI_SRS12) { + *reg &= ~val; + return; + } + + *reg = val; + + if (offset == SDHCI_SRS03) { + if (val & SDHCI_SRS03_DPS) { + /* Data-present command. Signal Transfer Complete only when the + * test asks for it. No error bits are set either way: this + * reproduces a wait timeout with a clean SRS12, which is what + * made the old code discard the error. */ + if (mock_complete_transfer) { + mock_regs[SDHCI_SRS12 / sizeof(uint32_t)] |= SDHCI_SRS12_TC; + } + } + else { + /* Command without data (e.g. CMD12 stop) completes normally */ + mock_regs[SDHCI_SRS04 / sizeof(uint32_t)] = (1U << 8); + mock_regs[SDHCI_SRS12 / sizeof(uint32_t)] |= SDHCI_SRS12_CC; + } + } +} + +void sdhci_platform_init(void) +{ +} + +void sdhci_platform_irq_init(void) +{ +} + +void sdhci_platform_set_bus_mode(int is_emmc) +{ + (void)is_emmc; +} + +#include "../../src/sdhci.c" + +/* Large enough to take the SDMA path (>= SDHCI_DMA_THRESHOLD) */ +#define TEST_XFER_SZ (8U * 1024U) + +static uint32_t test_buf[TEST_XFER_SZ / sizeof(uint32_t)]; + +static void reset_mock_state(int complete) +{ + memset(mock_regs, 0, sizeof(mock_regs)); + memset(test_buf, 0, sizeof(test_buf)); + mock_complete_transfer = complete; + g_mmc_irq_pending = 0; + g_mmc_irq_status = 0; +} + +/* The bug: SDMA never completes, SRS12 carries no error bit, and the CMD12 + * issued afterwards succeeds -- which used to overwrite the error. */ +START_TEST(test_sdma_timeout_is_reported) +{ + int ret; + + reset_mock_state(0); + ret = sdhci_read(MMC_CMD18_READ_MULTIPLE, 0, test_buf, TEST_XFER_SZ); + ck_assert_msg(ret != 0, + "sdhci_read must fail when the SDMA transfer never completes " + "(returned %d)", ret); +} +END_TEST + +/* Positive control: with Transfer Complete signalled the same path succeeds, + * so the assertions above are detecting the timeout and not simply a mock + * that can never succeed. */ +START_TEST(test_completed_transfer_succeeds) +{ + int ret; + + reset_mock_state(1); + ret = sdhci_read(MMC_CMD18_READ_MULTIPLE, 0, test_buf, TEST_XFER_SZ); + ck_assert_msg(ret == 0, + "sdhci_read must succeed when the transfer completes (returned %d)", + ret); +} +END_TEST + +Suite *sdhci_dma_suite(void) +{ + Suite *s = suite_create("sdhci-dma-error"); + TCase *tc = tcase_create("dma-error"); + + /* The timeout path spins a large retry count before giving up */ + tcase_set_timeout(tc, 60); + tcase_add_test(tc, test_sdma_timeout_is_reported); + tcase_add_test(tc, test_completed_transfer_succeeds); + suite_add_tcase(s, tc); + return s; +} + +int main(void) +{ + int fails; + SRunner *sr = srunner_create(sdhci_dma_suite()); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return (fails == 0) ? 0 : 1; +}