mirror of https://github.com/wolfSSL/wolfBoot.git
F-9735: roll back the 1.8V signaling switch when the SDHCI UHS retry fails
disk_read() called sdhci_uhs_recover() after any nonzero status from a data transfer. The recovery is a guess - no platform or retained state in wolfBoot identifies a warm-reset UHS condition, so on a plain 3.3V cold boot any first transfer failure (CRC, DMA, media, controller) switched the host to 1.8V signaling (SDHCI_SRS15_V18SE) without a CMD11 voltage negotiation. When the retry also failed, the voltage setting was never rolled back and g_uhs_recovered prevented further recovery: a transient error left the host - for the rest of the boot and the next stage - in a signaling state the card is not using. Make the guess self-correcting: if the retry of the failed chunk also fails, restore the previous signaling (clear V18SE, re-enable the SD clock) via sdhci_uhs_recover_rollback() before reporting the error. If the retry succeeds the switch stays - that is the warm-reset UHS case the recovery exists for. A later failure in a subsequent chunk no longer rolls back a configuration that is actually working. The unconditional break-on-error for eMMC builds is unchanged. Added tools/unit-tests/unit-sdhci-uhs-recover: it compiles the real src/sdhci.c (generated host copy with the three asm statements blanked and the real sdhci_read() renamed so the test can provide a scripted one) with register access backed by a host array, and asserts on the SDHCI_SRS15/SDHCI_SRS11 state for: a clean read (no switch), a failed first attempt with a successful retry (switch kept), a failed retry (roll back), and a later failure after a working recovery (switch kept). The rollback case fails pre-fix. Verified: unit-sdhci-uhs-recover 4/4 green post-fix (rollback case fails pre-fix), src/sdhci.c syntax-clean under DISK_EMMC, tools/unit-tests suite green (110 binaries).pull/862/head
Daniele Lacamera
parent
ceeaf2f24d
commit
d85127c173
39
src/sdhci.c
39
src/sdhci.c
|
|
@ -394,8 +394,14 @@ static int sdhci_set_power(uint32_t voltage)
|
|||
*
|
||||
* 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.
|
||||
* boot, and only after a transfer has already failed.
|
||||
*
|
||||
* Note: on a plain 3.3V cold boot this is a guess - any first transfer
|
||||
* failure (CRC, DMA, media, controller) triggers it. The guess is
|
||||
* self-correcting: the caller must undo the switch with
|
||||
* sdhci_uhs_recover_rollback() if the retry fails, so a wrong guess
|
||||
* leaves the host where it started rather than in a voltage state the
|
||||
* card is not using.
|
||||
*
|
||||
* Returns 0 if the switch was applied and the caller should retry. */
|
||||
static int sdhci_uhs_recover(void)
|
||||
|
|
@ -419,6 +425,21 @@ static int sdhci_uhs_recover(void)
|
|||
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* Undo a failed sdhci_uhs_recover(): the retry at 1.8V did not fix the
|
||||
* transfer, so this is not a warm-reset UHS condition. Restore 3.3V
|
||||
* signaling and the clock so the host is not left (for this boot and
|
||||
* the next stage) in a voltage state the card is not using. */
|
||||
static void sdhci_uhs_recover_rollback(void)
|
||||
{
|
||||
sdhci_reg_and(SDHCI_SRS11, ~SDHCI_SRS11_SDCE);
|
||||
|
||||
sdhci_reg_and(SDHCI_SRS15, ~SDHCI_SRS15_V18SE);
|
||||
udelay(5000); /* let the level shifter settle */
|
||||
|
||||
sdhci_reg_or(SDHCI_SRS11, SDHCI_SRS11_SDCE);
|
||||
udelay(1000);
|
||||
}
|
||||
#endif /* DISK_SDCARD */
|
||||
|
||||
/* ============================================================================
|
||||
|
|
@ -1739,6 +1760,9 @@ int disk_read(int drv, uint64_t start, uint32_t count, uint8_t *buf)
|
|||
uint32_t read_sz, block_addr;
|
||||
uint32_t tmp_block[SDHCI_BLOCK_SIZE/sizeof(uint32_t)];
|
||||
uint32_t start_offset = (start % SDHCI_BLOCK_SIZE);
|
||||
#ifdef DISK_SDCARD
|
||||
int uhs_switched = 0;
|
||||
#endif /* DISK_SDCARD */
|
||||
(void)drv; /* only one drive supported */
|
||||
|
||||
#ifdef DEBUG_SDHCI
|
||||
|
|
@ -1844,9 +1868,15 @@ int disk_read(int drv, uint64_t start, uint32_t count, uint8_t *buf)
|
|||
#endif
|
||||
}
|
||||
#ifdef DISK_SDCARD
|
||||
if (status != 0 && sdhci_uhs_recover() == 0) {
|
||||
if (status != 0 && !uhs_switched && sdhci_uhs_recover() == 0) {
|
||||
uhs_switched = 1;
|
||||
continue; /* retry this chunk with matched signaling */
|
||||
}
|
||||
if (status != 0 && uhs_switched) {
|
||||
/* The 1.8V retry failed: this is not a warm-reset UHS
|
||||
* condition, restore 3.3V signaling before giving up. */
|
||||
sdhci_uhs_recover_rollback();
|
||||
}
|
||||
#endif /* DISK_SDCARD */
|
||||
if (status != 0) {
|
||||
break;
|
||||
|
|
@ -1855,6 +1885,9 @@ int disk_read(int drv, uint64_t start, uint32_t count, uint8_t *buf)
|
|||
start += read_sz;
|
||||
buf += read_sz;
|
||||
count -= read_sz;
|
||||
#ifdef DISK_SDCARD
|
||||
uhs_switched = 0; /* chunk read cleanly */
|
||||
#endif /* DISK_SDCARD */
|
||||
}
|
||||
return status;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -105,6 +105,7 @@ TESTS+=unit-p1021-fcm-bytes
|
|||
TESTS+=unit-ls1028a-xspi-write
|
||||
TESTS+=unit-zynq-erase-loop
|
||||
TESTS+=unit-versal-qspi-dma
|
||||
TESTS+=unit-sdhci-uhs-recover
|
||||
|
||||
# The x86-64 EFI unit test needs the gnu-efi development headers (same
|
||||
# dependency as the CMake x86_64_efi target). Probe and only add the test
|
||||
|
|
@ -833,6 +834,22 @@ versal_host.c: ../../hal/versal.c versal_host.h
|
|||
unit-versal-qspi-dma: unit-versal-qspi-dma.c versal_host.c
|
||||
gcc -o $@ unit-versal-qspi-dma.c -DTARGET_versal -DEXT_FLASH -DARCH_AARCH64 -DARCH_64BIT $(CFLAGS) $(LDFLAGS)
|
||||
|
||||
# unit-sdhci-uhs-recover drives disk_read()'s UHS recovery path from the
|
||||
# real src/sdhci.c (F-9735: any read error permanently switched the host
|
||||
# to 1.8V signaling with no rollback). sdhci_host.c (generated below) is
|
||||
# identical to the real file except the three `asm volatile` statements
|
||||
# are blanked (they cannot assemble on x86) and the real sdhci_read() is
|
||||
# renamed to sdhci_read_hw() so the test can provide a scripted one. The
|
||||
# platform-provided sdhci_reg_read()/sdhci_reg_write() are backed by a
|
||||
# host register array; hal_get_timer_us() and wolfBoot_printf() are
|
||||
# stubbed in the test.
|
||||
|
||||
sdhci_host.c: ../../src/sdhci.c
|
||||
sed -E -e 's/^[[:space:]]*asm volatile\("[^"]*"[^;]*\);[[:space:]]*//' -e 's/^int sdhci_read\(/int sdhci_read_hw(/' $< > $@
|
||||
|
||||
unit-sdhci-uhs-recover: unit-sdhci-uhs-recover.c sdhci_host.c
|
||||
gcc -o $@ unit-sdhci-uhs-recover.c -DDISK_SDCARD $(CFLAGS) $(LDFLAGS)
|
||||
|
||||
# unit-efi-x86-open-image includes hal/x86_64_efi.c directly, with in-test
|
||||
# host mocks standing in for the gnu-efi runtime (BS, LibFileInfo, the
|
||||
# x86-64 efi_callN() trampolines). This is the only host build coverage
|
||||
|
|
|
|||
|
|
@ -0,0 +1,234 @@
|
|||
/* unit-sdhci-uhs-recover.c
|
||||
*
|
||||
* Regression test for F-9735: disk_read() called sdhci_uhs_recover()
|
||||
* after any nonzero status from a data transfer, which permanently
|
||||
* switched the host to 1.8V signaling (SDHCI_SRS15_V18SE) on a plain
|
||||
* 3.3V cold boot whenever the first transfer failed for any reason
|
||||
* (CRC, DMA, media, controller). If the retry also failed, the voltage
|
||||
* setting was never rolled back and g_uhs_recovered prevented further
|
||||
* recovery - so a transient error left the host with a signaling level
|
||||
* the card is not using, for the rest of the boot and the stage that
|
||||
* follows it.
|
||||
*
|
||||
* The test compiles the real src/sdhci.c (as sdhci_host.c, generated by
|
||||
* the Makefile). The generated copy differs from the real file only in:
|
||||
* - three `asm volatile` barrier/nop statements blanked (cannot
|
||||
* assemble on x86);
|
||||
* - the real sdhci_read() renamed to sdhci_read_hw() so this file can
|
||||
* provide a scripted sdhci_read() that stands in for the command /
|
||||
* data-transfer state machine.
|
||||
* Register access goes through the platform-provided sdhci_reg_read()/
|
||||
* sdhci_reg_write(), backed here by a host register array, so the test
|
||||
* asserts directly on the SDHCI_SRS15 / SDHCI_SRS11 state.
|
||||
*
|
||||
* 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
|
||||
*/
|
||||
|
||||
#include <check.h>
|
||||
#include <stdint.h>
|
||||
#include <stdio.h>
|
||||
#include <string.h>
|
||||
#include <stdarg.h>
|
||||
|
||||
#include "sdhci.h"
|
||||
#include "disk.h"
|
||||
|
||||
/* Host register file for the platform-provided register accessors. */
|
||||
static uint32_t g_sdhci_regs[0x400 / sizeof(uint32_t)];
|
||||
|
||||
uint32_t sdhci_reg_read(uint32_t offset)
|
||||
{
|
||||
return g_sdhci_regs[offset / sizeof(uint32_t)];
|
||||
}
|
||||
|
||||
void sdhci_reg_write(uint32_t offset, uint32_t val)
|
||||
{
|
||||
g_sdhci_regs[offset / sizeof(uint32_t)] = val;
|
||||
}
|
||||
|
||||
/* Timer for udelay(): advance on every read so busy loops terminate. */
|
||||
static uint64_t g_timer_us;
|
||||
uint64_t hal_get_timer_us(void)
|
||||
{
|
||||
return ++g_timer_us;
|
||||
}
|
||||
|
||||
void wolfBoot_printf(const char *format, ...)
|
||||
{
|
||||
(void)format;
|
||||
}
|
||||
|
||||
/* Platform hooks the SD init path touches; irrelevant to the recovery
|
||||
* logic under test. */
|
||||
void sdhci_platform_init(void)
|
||||
{
|
||||
}
|
||||
|
||||
void sdhci_platform_irq_init(void)
|
||||
{
|
||||
}
|
||||
|
||||
void sdhci_platform_set_bus_mode(int is_emmc)
|
||||
{
|
||||
(void)is_emmc;
|
||||
}
|
||||
|
||||
/* Scripted sdhci_read(): plays back g_read_script one entry per call.
|
||||
* 0 = success (fills dst with a per-block pattern), nonzero = failure. */
|
||||
#define MAX_READ_CALLS 32
|
||||
static int g_read_script[MAX_READ_CALLS];
|
||||
static int g_read_calls;
|
||||
static int g_script_len;
|
||||
|
||||
int sdhci_read(uint32_t cmd_index, uint32_t block_addr, uint32_t *dst,
|
||||
uint32_t sz)
|
||||
{
|
||||
int ret;
|
||||
uint32_t i;
|
||||
|
||||
(void)cmd_index;
|
||||
if (g_read_calls >= g_script_len)
|
||||
return -1;
|
||||
ret = g_read_script[g_read_calls];
|
||||
g_read_calls++;
|
||||
if (ret == 0) {
|
||||
for (i = 0; i < sz / sizeof(uint32_t); i++)
|
||||
dst[i] = (uint32_t)(0xA5 + block_addr + i);
|
||||
}
|
||||
return ret;
|
||||
}
|
||||
|
||||
/* The real HAL (identical to src/sdhci.c apart from the transforms
|
||||
* documented above). */
|
||||
#include "sdhci_host.c"
|
||||
|
||||
static void setup(void)
|
||||
{
|
||||
memset(g_sdhci_regs, 0, sizeof(g_sdhci_regs));
|
||||
g_timer_us = 0;
|
||||
g_read_calls = 0;
|
||||
g_script_len = 0;
|
||||
}
|
||||
|
||||
static void teardown(void)
|
||||
{
|
||||
}
|
||||
|
||||
static void script(const int *vals, int n)
|
||||
{
|
||||
if (n > MAX_READ_CALLS)
|
||||
n = MAX_READ_CALLS;
|
||||
memcpy(g_read_script, vals, (size_t)n * sizeof(int));
|
||||
g_script_len = n;
|
||||
}
|
||||
|
||||
/* A clean read must not touch the signaling level at all. */
|
||||
START_TEST(test_clean_read_no_switch)
|
||||
{
|
||||
static uint8_t __attribute__((aligned(4))) buf[SDHCI_BLOCK_SIZE];
|
||||
|
||||
script((int[]){ 0 }, 1);
|
||||
ck_assert_int_eq(disk_read(0, 0, sizeof(buf), buf), 0);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS15 / 4] & SDHCI_SRS15_V18SE, 0);
|
||||
}
|
||||
END_TEST
|
||||
|
||||
/* First transfer fails, the 1.8V retry succeeds: the switch stays (the
|
||||
* card really was left in UHS-I by a previous stage). */
|
||||
START_TEST(test_retry_success_keeps_switch)
|
||||
{
|
||||
static uint8_t __attribute__((aligned(4))) buf[SDHCI_BLOCK_SIZE];
|
||||
|
||||
script((int[]){ -1, 0 }, 2);
|
||||
ck_assert_int_eq(disk_read(0, 0, sizeof(buf), buf), 0);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS15 / 4] & SDHCI_SRS15_V18SE,
|
||||
SDHCI_SRS15_V18SE);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS11 / 4] & SDHCI_SRS11_SDCE,
|
||||
SDHCI_SRS11_SDCE);
|
||||
ck_assert_uint_eq(buf[0], 0xA5);
|
||||
}
|
||||
END_TEST
|
||||
|
||||
/* First transfer fails and the 1.8V retry fails too: this is not a
|
||||
* warm-reset UHS condition - restore 3.3V signaling and the clock so
|
||||
* the host is not left in a state the card is not using. */
|
||||
START_TEST(test_retry_failure_rolls_back)
|
||||
{
|
||||
static uint8_t __attribute__((aligned(4))) buf[SDHCI_BLOCK_SIZE];
|
||||
|
||||
script((int[]){ -1, -1 }, 2);
|
||||
ck_assert_int_eq(disk_read(0, 0, sizeof(buf), buf), -1);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS15 / 4] & SDHCI_SRS15_V18SE, 0);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS11 / 4] & SDHCI_SRS11_SDCE,
|
||||
SDHCI_SRS11_SDCE);
|
||||
}
|
||||
END_TEST
|
||||
|
||||
/* A later read fails after a recovery that is actually working: the
|
||||
* 1.8V configuration must be kept (rolling it back would break a card
|
||||
* that is really at 1.8V). Two disk_read() calls because the upper
|
||||
* layer reads in chunks. */
|
||||
START_TEST(test_later_failure_keeps_working_switch)
|
||||
{
|
||||
static uint8_t __attribute__((aligned(4))) buf1[SDHCI_BLOCK_SIZE];
|
||||
static uint8_t __attribute__((aligned(4))) buf2[SDHCI_BLOCK_SIZE];
|
||||
|
||||
/* chunk 1: first attempt fails, 1.8V retry works */
|
||||
script((int[]){ -1, 0 }, 2);
|
||||
ck_assert_int_eq(disk_read(0, 0, sizeof(buf1), buf1), 0);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS15 / 4] & SDHCI_SRS15_V18SE,
|
||||
SDHCI_SRS15_V18SE);
|
||||
|
||||
/* chunk 2: a later, unrelated failure - the working 1.8V config
|
||||
* must stay */
|
||||
script((int[]){ -1 }, 1);
|
||||
ck_assert_int_eq(disk_read(0, SDHCI_BLOCK_SIZE, sizeof(buf2), buf2),
|
||||
-1);
|
||||
ck_assert_int_eq(g_sdhci_regs[SDHCI_SRS15 / 4] & SDHCI_SRS15_V18SE,
|
||||
SDHCI_SRS15_V18SE);
|
||||
}
|
||||
END_TEST
|
||||
|
||||
Suite *sdhci_uhs_suite(void)
|
||||
{
|
||||
Suite *s = suite_create("sdhci-uhs-recover");
|
||||
TCase *tc = tcase_create("sdhci-uhs-recover");
|
||||
|
||||
tcase_add_checked_fixture(tc, setup, teardown);
|
||||
tcase_add_test(tc, test_clean_read_no_switch);
|
||||
tcase_add_test(tc, test_retry_success_keeps_switch);
|
||||
tcase_add_test(tc, test_retry_failure_rolls_back);
|
||||
tcase_add_test(tc, test_later_failure_keeps_working_switch);
|
||||
|
||||
suite_add_tcase(s, tc);
|
||||
return s;
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
int fails;
|
||||
Suite *s = sdhci_uhs_suite();
|
||||
SRunner *sr = srunner_create(s);
|
||||
|
||||
srunner_run_all(sr, CK_NORMAL);
|
||||
fails = srunner_ntests_failed(sr);
|
||||
srunner_free(sr);
|
||||
|
||||
return fails;
|
||||
}
|
||||
Loading…
Reference in New Issue