[ot] hw/opentitan: ot_uart: model watermark/empty as status interrupts#328
[ot] hw/opentitan: ot_uart: model watermark/empty as status interrupts#328moidx wants to merge 1 commit into
Conversation
tx_watermark, rx_watermark and tx_empty are declared as status-type interrupts in the UART HWIP (uart.hjson / prim_intr_hw IntrT="Status"): their INTR_STATE bits track the live FIFO condition every cycle rather than latching, and a RW1C write has no lasting effect while the condition holds. The model treated them as event-type, latching the bit on a receive batch and clearing it only via RW1C. As a result, once software drained the RX FIFO the latched rx_watermark bit stayed asserted, so the next received byte produced no fresh interrupt edge and interrupt-driven RX (e.g. a software-loopback test on ot-earlgrey) would hang. Compute the status-type bits live from the FIFO levels and OR them with the latched event-type bits when reading INTR_STATE and when driving the IRQ lines, so they assert and de-assert with the hardware condition: - rx_watermark: rx_fifo_depth >= threshold - tx_watermark: tx_fifo_depth < threshold - tx_empty: tx_fifo_depth == 0 Re-evaluate the lines after an RDATA pop and on FIFO_CTRL writes, and drop the now-unused tx_watermark edge-tracking state. Also align the FIFO depths (TxFifoDepth=32, RxFifoDepth=64) and the watermark threshold mappings (TX saturates at TxFifoDepth/2; RX saturates at RxFifoDepth-2, with rxilvl=7 disabling the interrupt) with uart_core.sv, and only push as many received bytes as the RX FIFO can hold. Signed-off-by: Miguel Osorio <miguelosorio@google.com>
luismarques
left a comment
There was a problem hiding this comment.
@jwnrt and I looked into this. We created some additional test cases, and in at least one case the behaviour of the OT_UART model is still not correct (double-checked by comparing the FPGA and QEMU executions), and will require follow-up improvements. The next step should be for us to produce a proper PR for those additional test cases. Still, this is a good improvement in correctness, so I think it can be merged, and follow-up commits can address the other scenarios we investigated. See the in-line feedback for some minor points, though.
| /* | ||
| * Status-type interrupts (tx_watermark, rx_watermark, tx_empty per uart.hjson). | ||
| * Unlike event-type interrupts, their INTR_STATE bits are not latched: the | ||
| * hardware drives them from the live FIFO condition every cycle, so a RW1C |
There was a problem hiding this comment.
Nit:
so a RW1C write has no lasting effect
From the docs for Status Type Interrupt:
INTR_STATE register, which has (ro) acccess permissions in this context.
So, putting it in terms of "no lasting effect" might not be quite the best phrasing, as you can't RW1C write it in the first place.
| switch (reg) { | ||
| case R_INTR_STATE: | ||
| val32 &= INTR_MASK; | ||
| s->regs[R_INTR_STATE] &= ~val32; /* RW1C */ |
There was a problem hiding this comment.
You're still writing to the overall register here as if it were rw1c in its entirety. Since some fields are rw1c and some are ro, and when you read R_INTR_STATE you get the combined R_INTR_STATE + live state, I think in some cases you might be able to observe a write to those ro fields, no? So, the safest and clearest option would probably be to mask those ro fields here, and only update the rw1c parts of R_INTR_STATE.
tx_watermark, rx_watermark and tx_empty are declared as status-type interrupts in the UART HWIP (uart.hjson / prim_intr_hw IntrT="Status"): their INTR_STATE bits track the live FIFO condition every cycle rather than latching, and a RW1C write has no lasting effect while the condition holds. The model treated them as event-type, latching the bit on a receive batch and clearing it only via RW1C. As a result, once software drained the RX FIFO the latched rx_watermark bit stayed asserted, so the next received byte produced no fresh interrupt edge and interrupt-driven RX (e.g. a software-loopback test on ot-earlgrey) would hang.
Compute the status-type bits live from the FIFO levels and OR them with the latched event-type bits when reading INTR_STATE and when driving the IRQ lines, so they assert and de-assert with the hardware condition:
Re-evaluate the lines after an RDATA pop and on FIFO_CTRL writes, and drop the now-unused tx_watermark edge-tracking state.
Also align the FIFO depths (TxFifoDepth=32, RxFifoDepth=64) and the watermark threshold mappings (TX saturates at TxFifoDepth/2; RX saturates at RxFifoDepth-2, with rxilvl=7 disabling the interrupt) with uart_core.sv, and only push as many received bytes as the RX FIFO can hold.