Re: [PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

Hi Bin,

Thanks a lot for the pointer -- I gave it a go. Let me share what I saw,
with a couple of caveats up front, since I'm not sure I tested it fairly.

Caveat 1: series 515264 doesn't apply on the tree I'm on -- it seems to
build on earlier SD/SDHCI rework I don't have yet (e.g.
SDHCIState.sdma_boundary_paused, sdhci_sdma_transfer_active, the Host
Control 2 migration), so patches 07-11 and 13 didn't build against my
base. So I couldn't test the series as-is.

Caveat 2: to still get a data point I hand-applied only what I understood
to be the core idea of patch 12 -- driving ADMA purely from the transfer
timer and no longer resuming it from MMIO reads/writes -- on top of my
AM64x tree. That's my approximation of your change, so I may well be
misrepresenting it; please correct me if so.

With that approximation, over 15 hands-off boots each of the same
unmodified image (a ~28 MiB CMD18 ADMA2-64 read), counting the U-Boot
"Timeout for status update" failures:

  baseline (sliced sdhci_do_adma, ADMA resumed from MMIO):    7/15 time out
  patch-12 approximation (ADMA timer-only, MMIO-decoupled):  10/15 time out
  my patch (complete non-INT chains in one pass):             0/15

I don't want to read too much into 7 vs 10 (small sample, overlapping),
but what I take from it is that the timer-only approximation still timed
out in most boots here, i.e. it doesn't seem to cover this particular
failure, whereas completing the chain in one pass avoided it in every run.

My guess as to why -- and this is only a guess -- is that the two things
might be different facets of the same symptom:

 - Your patch, as I understand it, targets a status *read* itself
   executing a pending ADMA batch and thus returning late.

 - What bites us on AM64x looks more like the *batched* transfer racing a
   guest-side timeout: sdhci_do_adma still does only
   SDHC_ADMA_DESCS_PER_DELAY descriptors per call and reschedules
   SDHC_TRANSFER_DELAY ns later on QEMU_CLOCK_VIRTUAL, so a big transfer
   is spread over many virtual-clock round-trips and U-Boot's timeout (in
   guest time) can expire mid-transfer. If that's right, keeping the
   per-batch reschedule would leave that race in place, which might be why
   the approximation didn't help here.

So my hunch is the two changes may be complementary rather than
alternatives -- but I could easily be wrong about your patch given I only
approximated it. Would it help if I rebased onto a base with the
prerequisite SD rework and tested the actual series, or if I shared the
AM64x reproducer? Whatever's most useful to you.

Thanks again,
Wadim