Skip to content

ot_spi_device: fix invalid IRQ signalling #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 3, 2025
Merged

ot_spi_device: fix invalid IRQ signalling #296

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 57 additions & 25 deletions hw/opentitan/ot_spi_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,23 @@ static bool ot_spi_device_flash_command_is_upload(const SpiDeviceFlash *f)
((f->cmd_info & CMD_INFO_UPLOAD_MASK) != 0u);
}

static bool ot_spi_device_flash_command_is_payload_en(const SpiDeviceFlash *f)
{
return (f->cmd_info & CMD_INFO_PAYLOAD_EN_MASK) != 0u;
Copy link

@AlexJones0 AlexJones0 Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if this is correct for the flash case. See usages of payload_en in spi_readcmd.sv in the RTL: if a non-standard lane-select value that isn't 0010/0011/1111 is selected then the SPI device just defaults to Single IO (0010). So there is no "disabling" the payload via payload_en.

However it looks like this is not the case for passthrough mode, which actually does care about the payload_en command info being non-zero - see spi_passthrough.sv.

As far as I can tell (it is very possible I am missing something, SPI device is very complex), nothing out of this readcmd and passthrough logic use this field - all other SPI modules tie it off. So its only use in QEMU emulation should probably be in the passthrough mode logic, whereas flash mode just ignores it given that we aren't emulating SPI data lanes here.

Copy link

@ziuziakowska ziuziakowska Dec 2, 2025

Choose a reason for hiding this comment

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

AFAIK this is true, I believe payload_en is used to control Passthrough mode, and in Flash mode the "payload" is handled by cmdparse and forwarded to the hardware modules which have their own state machine, or for the software commands it is uploaded if the upload field is set, so there is no "payload" for the downstream flash so-to-speak.

}

static bool ot_spi_device_flash_command_has_payload(const SpiDeviceFlash *f)
{
return ot_spi_device_is_sw_command(f->slot) &&
ot_spi_device_flash_command_is_payload_en(f);
}

static bool ot_spi_device_flash_command_is_rx_payload(const SpiDeviceFlash *f)
{
return ot_spi_device_is_sw_command(f->slot) &&
((f->cmd_info & CMD_INFO_PAYLOAD_DIR_MASK) == 0u);
}

static bool ot_spi_device_flash_is_readbuf_irq(const OtSPIDeviceState *s)
{
/*
Expand Down Expand Up @@ -879,6 +896,7 @@ static void ot_spi_device_release(OtSPIDeviceState *s)
bool update_irq = false;

OtSpiDeviceMode mode = ot_spi_device_get_mode(s);

switch (mode) {
case CTRL_MODE_FLASH:
case CTRL_MODE_PASSTHROUGH:
Expand All @@ -887,11 +905,6 @@ static void ot_spi_device_release(OtSPIDeviceState *s)
s->spi_regs[R_INTR_STATE] |= INTR_UPLOAD_CMDFIFO_NOT_EMPTY_MASK;
update_irq = true;
}
/* uploaded payload */
if (f->pos) {
s->spi_regs[R_INTR_STATE] |= INTR_UPLOAD_PAYLOAD_NOT_EMPTY_MASK;
update_irq = true;
}
/* update upload status register with payload information */
if ((mode == CTRL_MODE_FLASH && f->state == SPI_FLASH_UP_PAYLOAD) ||
(mode == CTRL_MODE_PASSTHROUGH &&
Expand All @@ -915,37 +928,52 @@ static void ot_spi_device_release(OtSPIDeviceState *s)
trace_ot_spi_device_flash_payload(s->ot_id, f->pos, payload_start,
payload_len);
}
/* passthrough mode: release CS */
if (mode == CTRL_MODE_PASSTHROUGH) {
ibex_irq_raise(&s->passthrough_cs);
/*
* "shows the last address accessed by the host system."
* "does not show the commands falling into the mailbox region or
* Read SFDP command’s address."
*/
if (f->slot >= SLOT_HW_READ_NORMAL && f->slot <= SLOT_HW_READ_QUAD_IO &&
!ot_spi_device_is_mailbox_match(s, f->last_read_addr)) {
trace_ot_spi_device_update_last_read_addr(s->ot_id,
f->last_read_addr);
s->spi_regs[R_LAST_READ_ADDR] = f->last_read_addr;
}
FLASH_CHANGE_STATE(s, IDLE);
break;
default:
break;
}

/*
* "shows the last address accessed by the host system."
* "does not show the commands falling into the mailbox region or
* Read SFDP command’s address."
*/
bool upload_intr = false;
switch (mode) {
case CTRL_MODE_FLASH:
upload_intr = ot_spi_device_is_sw_command(f->slot);
break;
case CTRL_MODE_PASSTHROUGH:
if (f->slot >= SLOT_HW_READ_NORMAL && f->slot <= SLOT_HW_READ_QUAD_IO &&
!ot_spi_device_is_mailbox_match(s, f->last_read_addr)) {
trace_ot_spi_device_update_last_read_addr(s->ot_id,
f->last_read_addr);
s->spi_regs[R_LAST_READ_ADDR] = f->last_read_addr;
/*
* @todo add support for payload interrupt management in Passthrough
* mode.
*/
if (f->pos) {
upload_intr = true;
}
/* passthrough mode: release CS */
ibex_irq_raise(&s->passthrough_cs);
break;
default:
break;
}

f->new_cmd = false;

if (upload_intr) {
if (f->pos) {
s->spi_regs[R_INTR_STATE] |= INTR_UPLOAD_PAYLOAD_NOT_EMPTY_MASK;
update_irq = true;
}
}

if (update_irq) {
ot_spi_device_update_irqs(s);
}
Expand Down Expand Up @@ -1264,7 +1292,7 @@ static uint8_t ot_spi_device_flash_read_buffer(OtSPIDeviceState *s)
f->pos++;
if (f->pos >= f->len) {
if (f->loop) {
f->pos = 0;
f->pos = 0u;
} else {
FLASH_CHANGE_STATE(s, DONE);
}
Expand Down Expand Up @@ -1349,12 +1377,13 @@ static uint8_t ot_spi_device_flash_read_data(OtSPIDeviceState *s)
return tx;
}

static void ot_spi_device_flash_init_payload(OtSPIDeviceState *s)
static void ot_spi_device_flash_init_upload(OtSPIDeviceState *s)
{
SpiDeviceFlash *f = &s->flash;

f->pos = 0;
f->len = SPI_SRAM_PAYLOAD_SIZE;
f->len =
ot_spi_device_flash_command_has_payload(f) ? SPI_SRAM_PAYLOAD_SIZE : 0u;
s->spi_regs[R_UPLOAD_STATUS2] = 0;
g_assert(f->payload);
FLASH_CHANGE_STATE(s, UP_PAYLOAD);
Expand All @@ -1373,7 +1402,7 @@ static void ot_spi_device_flash_decode_sw_command(OtSPIDeviceState *s)
f->len = 1u;
FLASH_CHANGE_STATE(s, UP_DUMMY);
} else if (ot_spi_device_flash_command_is_upload(f)) {
ot_spi_device_flash_init_payload(s);
ot_spi_device_flash_init_upload(s);
} else {
/*
* Any payload sent with a non-uploaded SW command is ignored. The
Expand Down Expand Up @@ -1409,7 +1438,7 @@ static void ot_spi_device_flash_exec_sw_command(OtSPIDeviceState *s, uint8_t rx)
f->len = 1u;
FLASH_CHANGE_STATE(s, UP_DUMMY);
} else if (ot_spi_device_flash_command_is_upload(f)) {
ot_spi_device_flash_init_payload(s);
ot_spi_device_flash_init_upload(s);
} else {
/*
* Any payload sent with a non-uploaded SW command is ignored.
Expand All @@ -1423,8 +1452,9 @@ static void ot_spi_device_flash_exec_sw_command(OtSPIDeviceState *s, uint8_t rx)
case SPI_FLASH_UP_DUMMY:
f->pos++;
g_assert(f->pos == f->len);
f->pos = 0u;
if (ot_spi_device_flash_command_is_upload(f)) {
ot_spi_device_flash_init_payload(s);
ot_spi_device_flash_init_upload(s);
} else {
/*
* Any payload sent with a non-uploaded SW command is ignored. The
Expand All @@ -1435,7 +1465,9 @@ static void ot_spi_device_flash_exec_sw_command(OtSPIDeviceState *s, uint8_t rx)
}
break;
case SPI_FLASH_UP_PAYLOAD:
f->payload[f->pos % SPI_SRAM_PAYLOAD_SIZE] = rx;
if (ot_spi_device_flash_command_is_rx_payload(f)) {
f->payload[f->pos % SPI_SRAM_PAYLOAD_SIZE] = rx;
}
f->pos++;
break;
case SPI_FLASH_DONE:
Expand Down
Loading