Am 5. Oktober 2026 10:46:08 UTC schrieb Bernhard Beschow <[email protected]>:
>
>
>Am 3. Oktober 2026 17:01:59 UTC schrieb Bin Meng <[email protected]>:
>>The SDHCI model processes the first ADMA descriptor batch inside the
>>command MMIO write, then resumes pending batches before every later
>>SDHCI MMIO read or write. This makes register access perform bulk DMA
>>work and can delay a status read even when completion bits are set.
>>
>>On the Icicle Kit, U-Boot uses ADMA2-64 and CMD18 to load a roughly
>>19 MiB FIT image from SD. It can report:
>>
>> Timeout for status update: 00000001 00000001
>> Timeout for status update: 00000003 00000001
>>
>>The status already contains the requested Command Complete bit, and may
>>also contain Transfer Complete. However, reading the interrupt status
>>first executes a pending ADMA batch. The MMIO read can therefore return
>>after U-Boot has reached its one-second polling deadline.
>>
>>Turn the existing transfer timer into an ADMA-only engine. Starting an
>>ADMA command arms the engine directly, and each timer callback runs one
>>bounded batch before yielding. MMIO reads and writes no longer perform
>>or schedule ADMA work. Keep PIO and SDMA synchronous.
>>
>>Cancel the engine and clear ADMA activity and error state on DATA or ALL
>>reset.
>>
>>Signed-off-by: Bin Meng <[email protected]>
>>---
>>
>>(no changes since v1)
>
>Hi Bin,
>
>I've accidentally left some comments on v2 after you sent v3:
>https://lore.kernel.org/qemu-devel/[email protected]/
> . Generally patch LGTM.
I've performed a dist upgrade inside a guest with ~600MB of download and ~2GB
of changed files with this patch cherry-picked on top of
https://lore.kernel.org/qemu-devel/[email protected]/ .
The guest rebooted successfully into the new environment. So:
Tested-by: Bernhard Beschow<[email protected]>
>
>Best regards,
>Bernhard
>
>>
>> hw/sd/sdhci.c | 143 ++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 87 insertions(+), 56 deletions(-)
>>
>>diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>index bd74c60f72..14c312a691 100644
>>--- a/hw/sd/sdhci.c
>>+++ b/hw/sd/sdhci.c
>>@@ -277,6 +277,7 @@ static void sdhci_set_inserted(DeviceState *dev, bool
>>level)
>> }
>> } else {
>> s->prnsts = 0x1fa0000;
>>+ timer_del(s->transfer_timer);
>> s->pwrcon &= ~SDHC_POWER_ON;
>> s->clkcon &= ~SDHC_CLOCK_SDCLK_EN;
>> if (s->norintstsen & SDHC_NISEN_REMOVE) {
>>@@ -348,7 +349,34 @@ static void sdhci_poweron_reset(DeviceState *dev)
>> }
>> }
>>
>>-static void sdhci_data_transfer(void *opaque);
>>+static void sdhci_data_transfer(SDHCIState *s);
>>+static void sdhci_adma_engine(void *opaque);
>>+
>>+static void sdhci_set_transfer_active(SDHCIState *s)
>>+{
>>+ s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
>>+ if (s->trnmod & SDHC_TRNS_READ) {
>>+ s->prnsts |= SDHC_DOING_READ;
>>+ } else {
>>+ s->prnsts |= SDHC_DOING_WRITE;
>>+ }
>>+}
>>+
>>+static void sdhci_schedule_adma(SDHCIState *s)
>>+{
>>+ timer_mod(s->transfer_timer,
>>+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_TRANSFER_DELAY);
>>+}
>>+
>>+static bool sdhci_adma_transfer_active(SDHCIState *s)
>>+{
>>+ return TRANSFERRING_DATA(s->prnsts) &&
>>+ (s->trnmod & SDHC_TRNS_DMA) &&
>>+ (s->cmdreg & SDHC_CMD_DATA_PRESENT) &&
>>+ sdhci_get_dma_type(s) != SDHC_CTRL_SDMA &&
>>+ !(s->admaerr & SDHC_ADMAERR_STATE_MASK) &&
>>+ !(s->errintsts & SDHC_EIS_ADMAERR);
>>+}
>>
>> #define BLOCK_SIZE_MASK (4 * KiB - 1)
>>
>>@@ -405,7 +433,13 @@ static void sdhci_send_command(SDHCIState *s)
>> if (!timeout && (s->blksize & BLOCK_SIZE_MASK) &&
>> (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>> s->data_count = 0;
>>- sdhci_data_transfer(s);
>>+ if ((s->trnmod & SDHC_TRNS_DMA) &&
>>+ sdhci_get_dma_type(s) != SDHC_CTRL_SDMA) {
>>+ sdhci_set_transfer_active(s);
>>+ sdhci_schedule_adma(s);
>>+ } else {
>>+ sdhci_data_transfer(s);
>>+ }
>> }
>> }
>>
>>@@ -850,7 +884,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr
>>*dscr)
>>
>> /* Advanced DMA data transfer */
>>
>>-static void sdhci_do_adma(SDHCIState *s)
>>+static void sdhci_adma_run_batch(SDHCIState *s)
>> {
>> unsigned int begin, length;
>> const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
>>@@ -956,15 +990,17 @@ static void sdhci_do_adma(SDHCIState *s)
>> }
>> if (res != MEMTX_OK) {
>> s->data_count = 0;
>>+ s->admaerr &= ~SDHC_ADMAERR_STATE_MASK;
>>+ s->admaerr |= SDHC_ADMAERR_STATE_ST_TFR;
>> if (s->errintstsen & SDHC_EISEN_ADMAERR) {
>> trace_sdhci_error("Set ADMA error flag");
>> s->errintsts |= SDHC_EIS_ADMAERR;
>> s->norintsts |= SDHC_NIS_ERR;
>> }
>> sdhci_update_irq(s);
>>- } else {
>>- s->admasysaddr += dscr.incr;
>>+ return;
>> }
>>+ s->admasysaddr += dscr.incr;
>> break;
>> case SDHC_ADMA_ATTR_ACT_LINK: /* link to next descriptor table */
>> s->admasysaddr = dscr.addr;
>>@@ -1012,50 +1048,52 @@ static void sdhci_do_adma(SDHCIState *s)
>> }
>>
>> /* we have unfinished business - reschedule to continue ADMA */
>>- timer_mod(s->transfer_timer,
>>- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>>SDHC_TRANSFER_DELAY);
>>+ sdhci_schedule_adma(s);
>> }
>>
>>-/* Perform data transfer according to controller configuration */
>>-
>>-static void sdhci_data_transfer(void *opaque)
>>+/* Run one independently scheduled, quota-bounded ADMA batch */
>>+static void sdhci_adma_engine(void *opaque)
>> {
>> SDHCIState *s = (SDHCIState *)opaque;
>>
>>- if (s->trnmod & SDHC_TRNS_DMA) {
>>- switch (sdhci_get_dma_type(s)) {
>>- case SDHC_CTRL_SDMA:
>>- sdhci_sdma_transfer(s);
>>- break;
>>- case SDHC_CTRL_ADMA1_32:
>>- if (!(s->capareg & R_SDHC_CAPAB_ADMA1_MASK)) {
>>- trace_sdhci_error("ADMA1 not supported");
>>- break;
>>- }
>>+ if (!sdhci_adma_transfer_active(s)) {
>>+ return;
>>+ }
>>
>>- sdhci_do_adma(s);
>>- break;
>>- case SDHC_CTRL_ADMA2_32:
>>- if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK)) {
>>- trace_sdhci_error("ADMA2 not supported");
>>- break;
>>- }
>>+ switch (sdhci_get_dma_type(s)) {
>>+ case SDHC_CTRL_ADMA1_32:
>>+ if (!(s->capareg & R_SDHC_CAPAB_ADMA1_MASK)) {
>>+ trace_sdhci_error("ADMA1 not supported");
>>+ return;
>>+ }
>>+ break;
>>+ case SDHC_CTRL_ADMA2_32:
>>+ if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK)) {
>>+ trace_sdhci_error("ADMA2 not supported");
>>+ return;
>>+ }
>>+ break;
>>+ case SDHC_CTRL_ADMA2_64:
>>+ if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK) ||
>>+ !(s->capareg & R_SDHC_CAPAB_BUS64BIT_MASK)) {
>>+ trace_sdhci_error("64 bit ADMA not supported");
>>+ return;
>>+ }
>>+ break;
>>+ default:
>>+ trace_sdhci_error("Unsupported ADMA type");
>>+ return;
>>+ }
>>
>>- sdhci_do_adma(s);
>>- break;
>>- case SDHC_CTRL_ADMA2_64:
>>- if (!(s->capareg & R_SDHC_CAPAB_ADMA2_MASK) ||
>>- !(s->capareg & R_SDHC_CAPAB_BUS64BIT_MASK)) {
>>- trace_sdhci_error("64 bit ADMA not supported");
>>- break;
>>- }
>>+ sdhci_adma_run_batch(s);
>>+}
>>
>>- sdhci_do_adma(s);
>>- break;
>>- default:
>>- trace_sdhci_error("Unsupported DMA type");
>>- break;
>>- }
>>+/* Perform synchronous PIO or SDMA data transfer */
>>+static void sdhci_data_transfer(SDHCIState *s)
>>+{
>>+ if (s->trnmod & SDHC_TRNS_DMA) {
>>+ assert(sdhci_get_dma_type(s) == SDHC_CTRL_SDMA);
>>+ sdhci_sdma_transfer(s);
>> } else {
>> if ((s->trnmod & SDHC_TRNS_READ) && sdbus_data_ready(&s->sdbus)) {
>> s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
>>@@ -1098,21 +1136,11 @@ sdhci_buff_access_is_sequential(SDHCIState *s,
>>unsigned byte_num)
>> return true;
>> }
>>
>>-static void sdhci_resume_pending_transfer(SDHCIState *s)
>>-{
>>- timer_del(s->transfer_timer);
>>- sdhci_data_transfer(s);
>>-}
>>-
>> static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>> {
>> SDHCIState *s = (SDHCIState *)opaque;
>> uint32_t ret = 0;
>>
>>- if (timer_pending(s->transfer_timer)) {
>>- sdhci_resume_pending_transfer(s);
>>- }
>>-
>> switch (offset & ~0x3) {
>> case SDHC_SYSAD:
>> ret = s->sdmasysad;
>>@@ -1235,7 +1263,10 @@ static inline void sdhci_reset_write(SDHCIState *s,
>>uint8_t value)
>> s->norintsts &= ~SDHC_NIS_CMDCMP;
>> break;
>> case SDHC_RESET_DATA:
>>+ timer_del(s->transfer_timer);
>> s->data_count = 0;
>>+ s->admaerr = 0;
>>+ s->errintsts &= ~SDHC_EIS_ADMAERR;
>> s->prnsts &= ~(SDHC_SPACE_AVAILABLE | SDHC_DATA_AVAILABLE |
>> SDHC_DOING_READ | SDHC_DOING_WRITE |
>> SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE);
>>@@ -1244,6 +1275,10 @@ static inline void sdhci_reset_write(SDHCIState *s,
>>uint8_t value)
>> s->sdma_boundary_paused = false;
>> s->norintsts &= ~(SDHC_NIS_WBUFRDY | SDHC_NIS_RBUFRDY |
>> SDHC_NIS_DMA | SDHC_NIS_TRSCMP | SDHC_NIS_BLKGAP);
>>+ if (!s->errintsts) {
>>+ s->norintsts &= ~SDHC_NIS_ERR;
>>+ }
>>+ sdhci_update_irq(s);
>> break;
>> }
>> }
>>@@ -1257,10 +1292,6 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
>>unsigned size)
>> uint32_t value = val;
>> value <<= shift;
>>
>>- if (timer_pending(s->transfer_timer)) {
>>- sdhci_resume_pending_transfer(s);
>>- }
>>-
>> switch (offset & ~0x3) {
>> case SDHC_SYSAD:
>> if (!TRANSFERRING_DATA(s->prnsts) ||
>>@@ -1521,7 +1552,7 @@ void sdhci_initfn(SDHCIState *s)
>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> sdhci_raise_insertion_irq, s);
>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>- sdhci_data_transfer, s);
>>+ sdhci_adma_engine, s);
>>
>> s->io_ops = &sdhci_mmio_ops;
>> }
