From 2015312bc75a71a6a9d9d01ccfe832ea49203a24 Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Thu, 13 Nov 2025 10:33:30 +0000 Subject: [PATCH 1/3] [ot] hw/opentitan: ot_spi_device: Fix LAST_READ_ADDR write This register is read-only, it should not be writable. Signed-off-by: Alex Jones --- hw/opentitan/ot_spi_device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/opentitan/ot_spi_device.c b/hw/opentitan/ot_spi_device.c index 8910f5a058af3..cd3818443af8c 100644 --- a/hw/opentitan/ot_spi_device.c +++ b/hw/opentitan/ot_spi_device.c @@ -2085,7 +2085,6 @@ static void ot_spi_device_spi_regs_write(void *opaque, hwaddr addr, val32 &= R_READ_THRESHOLD_THRESHOLD_MASK; s->spi_regs[reg] = val32; break; - case R_LAST_READ_ADDR: case R_MAILBOX_ADDR: case R_CMD_FILTER_0: case R_CMD_FILTER_1: @@ -2135,6 +2134,7 @@ static void ot_spi_device_spi_regs_write(void *opaque, hwaddr addr, val32 &= CMD_INFO_SPC_MASK; s->spi_regs[reg] = val32; break; + case R_LAST_READ_ADDR: case R_STATUS: case R_UPLOAD_STATUS: case R_UPLOAD_STATUS2: From 9f6056617ba198c85a2db1803b32335035d796b4 Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Thu, 13 Nov 2025 10:34:20 +0000 Subject: [PATCH 2/3] [ot] hw/opentitan: ot_spi_device: Invert CSb status The SPI Device's `STATUS.csb` field is the "Direct input of CSb signal". The current logic reports a 1 if the CS is active (i.e. we are not idle or in some error state, the SPI bus is active), but /CS is active low and as such it should report a 0 when active, and a 1 when not. Signed-off-by: Alex Jones --- hw/opentitan/ot_spi_device.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/opentitan/ot_spi_device.c b/hw/opentitan/ot_spi_device.c index cd3818443af8c..970b71e28d66c 100644 --- a/hw/opentitan/ot_spi_device.c +++ b/hw/opentitan/ot_spi_device.c @@ -773,10 +773,12 @@ static uint32_t ot_spi_device_get_status(const OtSPIDeviceState *s) { uint32_t status = 0u; - if (ot_spi_device_is_cs_active(s)) { + if (!ot_spi_device_is_cs_active(s)) { status |= R_STATUS_CSB_MASK; } + /* @todo: Add TPM CSB when the TPM CS is added to the SpiDev protocol */ + return status; } From 0c38169b84e98083bcd1c892839ad6a4fc35955b Mon Sep 17 00:00:00 2001 From: Alex Jones Date: Thu, 13 Nov 2025 10:37:06 +0000 Subject: [PATCH 3/3] [ot] hw/opentitan: ot_spi_device: Reduce arbitrary flipbuf pacing See the relevant comment - this arbitrary timeout is much longer than expected and can cause unexpected timeouts when using the SPI device for large reads with software that does not expect to handle this event and fill these buffers. When using an icount shift of 6, we can often end up waiting just over a second for the read to resume, greatly slowing down SPI operation. Signed-off-by: Alex Jones --- hw/opentitan/ot_spi_device.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/hw/opentitan/ot_spi_device.c b/hw/opentitan/ot_spi_device.c index 970b71e28d66c..e8479f8297516 100644 --- a/hw/opentitan/ot_spi_device.c +++ b/hw/opentitan/ot_spi_device.c @@ -224,21 +224,24 @@ REG32(TPM_READ_FIFO, 0x34u) #define SPI_BUS_HEADER_SIZE (2u * sizeof(uint32_t)) #define SPI_TPM_READ_FIFO_SIZE_BYTES \ (PARAM_TPM_RD_FIFO_DEPTH * sizeof(uint32_t)) -/** - * Delay for handling non-aligned generic data transfer and flush the FIFO. - * Generic mode is deprecated anyway. Arbitrarily set to 1 ms. - */ -#define SPI_BUS_TIMEOUT_NS 1000000u /* * Pacing time to give hand back to the vCPU when a readbuf event is triggered. - * The scheduler timer tell the CharDev backend not to consume (nor push back) - * any more bytes from/to the SPI bus. The timer can either exhausts on its own, - * which should never happen, and much more likely when the readbuf interruption - * is cleared by the guest SW, which should usually happen once the SW has - * filled in the read buffer. As soon as the timer is cancelled/over, the - * CharDev resumes its SPI bus bytestream management. Arbitrarily set to 100 ms. + * This is needed so guest SW can respond to the interrupt and fill the buffer. + * + * The scheduled timer tell the CharDev backend not to consume (nor push back) + * any more bytes from/to the SPI bus. The Chardev will resume its SPI bus + * bytestream management as soon as the timer is cancelled/expires. Ideally, + * guest SW will clear the readbuf interrupt causing the timer to expire, + * usually once SW has filled in the read buffer. If Guest SW does not use this + * (e.g. it writes across both buffers in advance and does not expect to handle + * a buffer flip event) then this will not happen, and the timer must exhaust + * on its own, thus we only set this delay to a small arbitrary value of 10 ms. + * + * @todo: A better approach might be to yield to the vCPU every few bytes/words + * to better emulate the concurrency of large SPI transactions with SW, + * potentially keeping a small additional delay on a buffer flip. */ -#define SPI_BUS_FLASH_READ_DELAY_NS 100000000u +#define SPI_BUS_FLASH_READ_DELAY_NS 10000000 /* * Memory layout extracted from the documentation: @@ -965,10 +968,10 @@ static void ot_spi_device_flash_pace_spibus(OtSPIDeviceState *s) SpiDeviceFlash *f = &s->flash; timer_del(f->irq_timer); - uint64_t now = qemu_clock_get_ns(OT_VIRTUAL_CLOCK); + int64_t now = qemu_clock_get_ns(OT_VIRTUAL_CLOCK); trace_ot_spi_device_flash_pace(s->ot_id, "set", timer_pending(f->irq_timer)); - timer_mod(f->irq_timer, (int64_t)(now + SPI_BUS_FLASH_READ_DELAY_NS)); + timer_mod(f->irq_timer, now + SPI_BUS_FLASH_READ_DELAY_NS); } static bool