From f114666b428cbaf6ecfc9583dd01f9fd0ba2d234 Mon Sep 17 00:00:00 2001 From: MCKero Date: Fri, 28 Aug 2026 15:05:15 +0100 Subject: [PATCH] Start DMA on the peripheral's request, and clock both directions together Two related faults in the DMA model, both of which corrupted flash reads. Transfers started when a channel was enabled. On hardware, enabling only arms a channel; the transfer begins when the peripheral raises its DMA request. The flash driver's SPI_ReadBuf arms RX, arms TX, then enables SPI and sets TXDMAEN -- so firing at arm time clocked the bus before the read command had been sent. SPI now kicks the armed channels from CR1/CR2 when SPE and a DMA request enable are both set, which covers the read path (SPE last) and the write path alike. Each channel also ran to completion independently. SPI is duplex: one clocked byte is simultaneously sent and received, and the driver relies on that, pairing a memory-to-peripheral channel feeding dummy bytes with a peripheral-to-memory channel collecting the reply. Running them in sequence meant TX clocked the whole transfer out before RX looked at the bus, so RX collected nothing. They are now stepped together, one byte at a time. Either fault alone made a 4 KB sector read return zeros. PY25Q16_WriteBuffer reads a sector into SectorCache, patches it, and writes the whole thing back, so a zeroed read turned into a zeroed sector -- including the per-band VFO frequencies at 0x9000. That is a second, independent cause of typed frequencies reverting to 18 MHz, on top of the missing page wrap fixed in da1ad7e. Verified: the frequency area stays 0xFF across a boot where it was previously zeroed every time, and an instrumented build shows the sector read now returning 0xFF rather than 0x00. test_flash_persist.py now starts from the pristine image rather than assets/flash.img. A dirty live image left nothing for the boot to write, which surfaced as "the image is byte-identical" -- a failure that looks like broken persistence but is really a dirty fixture. Runs twice in a row cleanly now. keypad_test.py and the 141 unit tests still pass. --- qemu/py32f071.c | 160 +++++++++++++++++++++++++++--------- tools/test_flash_persist.py | 14 +++- 2 files changed, 135 insertions(+), 39 deletions(-) diff --git a/qemu/py32f071.c b/qemu/py32f071.c index 8247796..3e82640 100644 --- a/qemu/py32f071.c +++ b/qemu/py32f071.c @@ -623,6 +623,14 @@ struct PY32SpiState { PY32SpiXferFn xfer; void *xfer_opaque; + + /* + * Set by the DMA model so SPI can kick armed channels when the guest asserts + * a DMA request. Without this the request is invisible to DMA and the + * transfer has to be started at arm time, which is too early. + */ + void (*dma_kick)(void *dma, PY32SpiState *spi); + void *dma; }; #define SPI_CR1 0x00 @@ -634,6 +642,10 @@ struct PY32SpiState { #define SPI_SR_TXE (1u << 1) #define SPI_SR_BSY (1u << 7) +#define SPI_CR1_SPE (1u << 6) /* SPI enable */ +#define SPI_CR2_RXDMAEN (1u << 0) /* RX DMA request enable */ +#define SPI_CR2_TXDMAEN (1u << 1) /* TX DMA request enable */ + void py32_spi_set_xfer(PY32SpiState *s, PY32SpiXferFn fn, void *opaque); void py32_spi_set_xfer(PY32SpiState *s, PY32SpiXferFn fn, void *opaque) @@ -672,8 +684,31 @@ static void py32_spi_write(void *opaque, hwaddr addr, uint64_t value, unsigned s PY32SpiState *s = opaque; switch (addr) { - case SPI_CR1: s->cr1 = value; break; - case SPI_CR2: s->cr2 = value; break; + case SPI_CR1: + s->cr1 = value; + /* + * SPE can be the last thing enabled. The flash driver's read path arms both + * channels, sets RXDMAEN, enables SPI, then sets TXDMAEN -- but the write + * path enables SPI last, so both orders have to work. + */ + if ((value & SPI_CR1_SPE) && (s->cr2 & (SPI_CR2_TXDMAEN | SPI_CR2_RXDMAEN)) + && s->dma_kick) { + s->dma_kick(s->dma, s); + } + break; + case SPI_CR2: + s->cr2 = value; + /* + * A DMA request is what actually starts the transfer on hardware. Kick the + * armed channels here rather than when they were enabled: at arm time the + * read command has not been clocked out yet, so the reply would be read + * before the device had anything to say. + */ + if ((value & (SPI_CR2_TXDMAEN | SPI_CR2_RXDMAEN)) + && (s->cr1 & SPI_CR1_SPE) && s->dma_kick) { + s->dma_kick(s->dma, s); + } + break; case SPI_SR: /* Flags are mostly hardware-driven; keep TXE asserted. */ s->sr = (value & ~SPI_SR_TXE) | SPI_SR_TXE; @@ -833,52 +868,86 @@ static PY32SpiState *py32_dma_spi_for(PY32DmaState *s, uint32_t paddr) } /* - * Runs a channel to completion. Transmit channels feed bytes to the device; - * receive channels store what it returns. When both directions are armed the - * transmit side has usually been enabled first, and the flash driver arms RX - * before TX, so a receive channel drives the clock itself -- otherwise nothing - * would ever be shifted in. + * Run the armed channels for one SPI peripheral. + * + * SPI is inherently duplex: every clocked byte simultaneously sends one byte and + * receives one. The firmware exploits this, arming a memory-to-peripheral channel + * that feeds dummy bytes and a peripheral-to-memory channel that collects the + * reply, both over the same transfer. + * + * So the two channels have to be stepped together, one byte at a time. Running + * them one after another -- as this did when each channel started on its own + * enable -- means the TX channel clocks the entire transfer out before the RX + * channel ever looks at the bus, and RX collects nothing. */ -static void py32_dma_run(PY32DmaState *s, int ch) +static void py32_dma_run_for_spi(PY32DmaState *s, PY32SpiState *spi) { - PY32DmaChannel *c = &s->ch[ch]; - PY32SpiState *spi = py32_dma_spi_for(s, c->cpar); + AddressSpace *as = &address_space_memory; + int tx = -1, rx = -1; - if (!spi || c->cndtr == 0) { - /* Nothing attached, or a zero-length transfer: report completion so the - * guest does not wait forever. */ - s->isr |= DMA_FLAG_TCIF(ch) | DMA_FLAG_GIF(ch); - c->cndtr = 0; - py32_dma_update_irq(s); + for (int ch = 0; ch < PY32_DMA_CHANNELS; ch++) { + PY32DmaChannel *c = &s->ch[ch]; + if (!(c->ccr & DMA_CCR_EN) || c->cndtr == 0) { + continue; + } + if (py32_dma_spi_for(s, c->cpar) != spi) { + continue; + } + if (c->ccr & DMA_CCR_DIR) { + tx = ch; + } else { + rx = ch; + } + } + + if (tx < 0 && rx < 0) { return; } - const bool from_memory = (c->ccr & DMA_CCR_DIR) != 0; - const bool minc = (c->ccr & DMA_CCR_MINC) != 0; - uint32_t maddr = c->cmar; - AddressSpace *as = &address_space_memory; + /* Length is whichever side is armed; when both are, they match. */ + uint32_t count = tx >= 0 ? s->ch[tx].cndtr : s->ch[rx].cndtr; + uint32_t tx_addr = tx >= 0 ? s->ch[tx].cmar : 0; + uint32_t rx_addr = rx >= 0 ? s->ch[rx].cmar : 0; + const bool tx_inc = tx >= 0 && (s->ch[tx].ccr & DMA_CCR_MINC); + const bool rx_inc = rx >= 0 && (s->ch[rx].ccr & DMA_CCR_MINC); - while (c->cndtr > 0) { - uint8_t byte = 0xff; + while (count > 0) { + uint8_t out = 0xff; - if (from_memory) { - address_space_read(as, maddr, MEMTXATTRS_UNSPECIFIED, &byte, 1); - (void)py32_spi_xfer_byte(spi, byte); - } else { - byte = py32_spi_xfer_byte(spi, 0xff); - address_space_write(as, maddr, MEMTXATTRS_UNSPECIFIED, &byte, 1); + if (tx >= 0) { + address_space_read(as, tx_addr, MEMTXATTRS_UNSPECIFIED, &out, 1); } - if (minc) { - maddr++; + const uint8_t in = py32_spi_xfer_byte(spi, out); + + if (rx >= 0) { + address_space_write(as, rx_addr, MEMTXATTRS_UNSPECIFIED, &in, 1); } - c->cndtr--; + + if (tx_inc) { + tx_addr++; + } + if (rx_inc) { + rx_addr++; + } + count--; } - s->isr |= DMA_FLAG_TCIF(ch) | DMA_FLAG_GIF(ch); + for (int ch = 0; ch < PY32_DMA_CHANNELS; ch++) { + if (ch == tx || ch == rx) { + s->ch[ch].cndtr = 0; + s->isr |= DMA_FLAG_TCIF(ch) | DMA_FLAG_GIF(ch); + } + } py32_dma_update_irq(s); } +/* Thin adaptor so SPI can call into DMA without knowing its type. */ +static void py32_dma_kick(void *dma, PY32SpiState *spi) +{ + py32_dma_run_for_spi((PY32DmaState *)dma, spi); +} + static uint64_t py32_dma_read(void *opaque, hwaddr addr, unsigned size) { PY32DmaState *s = opaque; @@ -929,12 +998,21 @@ static void py32_dma_write(void *opaque, hwaddr addr, uint64_t value, unsigned s case DMA_CPAR: s->ch[ch].cpar = value; break; case DMA_CMAR: s->ch[ch].cmar = value; break; case DMA_CCR: { - const bool was_enabled = (s->ch[ch].ccr & DMA_CCR_EN) != 0; - s->ch[ch].ccr = value; - if (!was_enabled && (value & DMA_CCR_EN)) { - py32_dma_run(s, ch); - } - break; + s->ch[ch].ccr = value; + /* + * Enabling a channel only arms it. On real hardware the transfer starts + * when the peripheral raises its DMA request, which for SPI means + * SPI_CR2's TXDMAEN. Running it here instead broke duplex reads: the + * firmware's SPI_ReadBuf arms RX then TX and only then enables SPI, so a + * transfer that fired at arm time clocked the bus before the read command + * had been sent, and the destination buffer came back as zeros. + * + * That is what wiped the VFO frequency area. PY25Q16_WriteBuffer reads the + * whole 4 KB sector into SectorCache, patches it, and writes it back; the + * read returned zeros, so the write-back filled the sector with zeros -- + * including the per-band frequencies at 0x9000. + */ + break; } default: break; @@ -1652,6 +1730,12 @@ static void py32f071_soc_realize(DeviceState *dev_soc, Error **errp) */ s->dma.spi[0] = &s->spi[0]; s->dma.spi[1] = &s->spi[1]; + + /* And the reverse link, so a DMA request from SPI can start the transfer. */ + for (int i = 0; i < 2; i++) { + s->spi[i].dma = &s->dma; + s->spi[i].dma_kick = py32_dma_kick; + } if (!sysbus_realize(SYS_BUS_DEVICE(&s->dma), errp)) { return; } diff --git a/tools/test_flash_persist.py b/tools/test_flash_persist.py index cd315b4..c941937 100755 --- a/tools/test_flash_persist.py +++ b/tools/test_flash_persist.py @@ -149,7 +149,19 @@ def main(): workdir = tempfile.mkdtemp(prefix="uvk5-persist-") image = os.path.join(workdir, "flash.img") - shutil.copy(SOURCE_IMAGE, image) + + # Start from the pristine image, not assets/flash.img. The live image may have + # been written by an earlier session, and then a boot has nothing left to save + # -- which shows up as "the image is byte-identical", a confusing failure that + # looks like persistence is broken when it is the fixture that is dirty. + pristine_gz = os.path.join(os.path.dirname(SOURCE_IMAGE), + "pristine", "flash-pristine.img.gz") + if os.path.exists(pristine_gz): + import gzip + with gzip.open(pristine_gz, "rb") as src, open(image, "wb") as dst: + shutil.copyfileobj(src, dst) + else: + shutil.copy(SOURCE_IMAGE, image) failures = [] try: