Re: [PATCH 12/26] hw/sd: sdhci: Run ADMA independently of MMIO

On 23/7/26 17:18, Bin Meng wrote:
> 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]>
> ---
> 
>   hw/sd/sdhci.c | 140 +++++++++++++++++++++++++++++++-------------------
>   1 file changed, 86 insertions(+), 54 deletions(-)


> -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,12 +990,15 @@ 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);
> +                return;
>               } else {
>                   s->admasysaddr += dscr.incr;
>               }

This now reads better as "if block to handle error and return":

-- >8 --
@@ -999,9 +999,8 @@ static void sdhci_adma_run_batch(SDHCIState *s)
                  }
                  sdhci_update_irq(s);
                  return;
-            } else {
-                s->admasysaddr += dscr.incr;
              }
+            s->admasysaddr += dscr.incr;
              break;
---