Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions hw/opentitan/ot_spi_device.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that could be a better alternative; however would need to be carefully measured with several kind of hosts, as it might come with a large overhead, depending on the host.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - I do not know how common this pattern of guest SW relying on concurrent operation with the SPI Host buffer reads is but it would not be ideal to slow down general SPI Host usage because of it. It would need to be more carefully thought out (hence the todo 😅)

* 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:
Expand Down Expand Up @@ -773,10 +776,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)) {
Comment on lines -776 to +779

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that TPM mode has been implemented there is also the STATUS.tpm_csb bit which is also active low: Hardware Interfaces, so maybe this function should be inlined here and extended to also do that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though TPM mode is now implemented, the TPM CS itself is not (we only have 1 CS in the protocol currently) - see relevant discussion here.

I think that it's a good idea to make sure this isn't lost though, so I've added a TODO capturing this missing field.

status |= R_STATUS_CSB_MASK;
}

/* @todo: Add TPM CSB when the TPM CS is added to the SpiDev protocol */

return status;
}

Expand Down Expand Up @@ -963,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
Expand Down Expand Up @@ -2085,7 +2090,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:
Expand Down Expand Up @@ -2135,6 +2139,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:
Expand Down
Loading