Skip to content

Commit 5013d60

Browse files
committed
[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 <alex.jones@lowrisc.org>
1 parent 5073aa7 commit 5013d60

File tree

1 file changed

+17
-14
lines changed

1 file changed

+17
-14
lines changed

hw/opentitan/ot_spi_device.c

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -224,21 +224,24 @@ REG32(TPM_READ_FIFO, 0x34u)
224224
#define SPI_BUS_HEADER_SIZE (2u * sizeof(uint32_t))
225225
#define SPI_TPM_READ_FIFO_SIZE_BYTES \
226226
(PARAM_TPM_RD_FIFO_DEPTH * sizeof(uint32_t))
227-
/**
228-
* Delay for handling non-aligned generic data transfer and flush the FIFO.
229-
* Generic mode is deprecated anyway. Arbitrarily set to 1 ms.
230-
*/
231-
#define SPI_BUS_TIMEOUT_NS 1000000u
232227
/*
233228
* Pacing time to give hand back to the vCPU when a readbuf event is triggered.
234-
* The scheduler timer tell the CharDev backend not to consume (nor push back)
235-
* any more bytes from/to the SPI bus. The timer can either exhausts on its own,
236-
* which should never happen, and much more likely when the readbuf interruption
237-
* is cleared by the guest SW, which should usually happen once the SW has
238-
* filled in the read buffer. As soon as the timer is cancelled/over, the
239-
* CharDev resumes its SPI bus bytestream management. Arbitrarily set to 100 ms.
229+
* This is needed so guest SW can respond to the interrupt and fill the buffer.
230+
*
231+
* The scheduled timer tell the CharDev backend not to consume (nor push back)
232+
* any more bytes from/to the SPI bus. The Chardev will resume its SPI bus
233+
* bytestream management as soon as the timer is cancelled/expires. Ideally,
234+
* guest SW will clear the readbuf interrupt causing the timer to expire,
235+
* usually once SW has filled in the read buffer. If Guest SW does not use this
236+
* (e.g. it writes across both buffers in advance and does not expect to handle
237+
* a buffer flip event) then this will not happen, and the timer must exhaust
238+
* on its own, thus we only set this delay to a small arbitrary value of 10 ms.
239+
*
240+
* @todo: A better approach might be to yield to the vCPU every few bytes/words
241+
* to better emulate the concurrency of large SPI transactions with SW,
242+
* potentially keeping a small additional delay on a buffer flip.
240243
*/
241-
#define SPI_BUS_FLASH_READ_DELAY_NS 100000000u
244+
#define SPI_BUS_FLASH_READ_DELAY_NS 10000000
242245

243246
/*
244247
* Memory layout extracted from the documentation:
@@ -965,10 +968,10 @@ static void ot_spi_device_flash_pace_spibus(OtSPIDeviceState *s)
965968
SpiDeviceFlash *f = &s->flash;
966969

967970
timer_del(f->irq_timer);
968-
uint64_t now = qemu_clock_get_ns(OT_VIRTUAL_CLOCK);
971+
int64_t now = qemu_clock_get_ns(OT_VIRTUAL_CLOCK);
969972
trace_ot_spi_device_flash_pace(s->ot_id, "set",
970973
timer_pending(f->irq_timer));
971-
timer_mod(f->irq_timer, (int64_t)(now + SPI_BUS_FLASH_READ_DELAY_NS));
974+
timer_mod(f->irq_timer, now + SPI_BUS_FLASH_READ_DELAY_NS);
972975
}
973976

974977
static bool

0 commit comments

Comments
 (0)