Skip to content

Conversation

@engdoreis
Copy link

@engdoreis engdoreis commented Oct 22, 2025

ThIs PR does not cover all the TPM implementation, the HW registers will be covered by a follow up PR.

To test use the Opentitan PR lowRISC/opentitan#28542 and run:

bazel test --test_output=streamed --build_tests_only \
    $(bazel query //sw/device/tests:all | grep qemu | grep spi_device) \
    --test_tag_filters="qemu" --override_repository="+qemu+qemu_opentitan=$qemu_repo"

@rivos-eblot
Copy link

I'll review it soon; I've noticed there are a couple of type mismatches (e.g. bool x; ... x = 1;) and missing unsigned constant suffixes (u)

@rivos-eblot
Copy link

see also the PR title, as they now follow the

`component`: ...

formatting, i.e. ot_spi_device: Implement the TPM mode

Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

I got part way through reviewing but then @engdoreis told me this is still being actively worked on as a draft. I'll leave the comments I had for now though to aid development, but I haven't looked at any of the TPM logic itself yet.

@engdoreis engdoreis changed the title [DRAFT][OT spi_device] Implement the TPM mode [ot] hw/opentitan: ot_spi_device: Implement the TPM mode Oct 22, 2025
@engdoreis engdoreis marked this pull request as ready for review October 22, 2025 16:28
@engdoreis engdoreis force-pushed the ot_spi_tpm branch 4 times, most recently from 8675744 to aa54714 Compare October 23, 2025 15:03
Copy link

@rivos-eblot rivos-eblot left a comment

Choose a reason for hiding this comment

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

Note: I would recommend to add new trace_ot_spi_device_...
Traces are very useful for debugging the emulation itself, but they are also very useful when developping guest SW (RISC-V code) and remote host peer SW. Thanks.

Copy link

@rivos-eblot rivos-eblot left a comment

Choose a reason for hiding this comment

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

Two remaining items:

  • static assertion location
  • emptying TPM FIFO @ reset.

@engdoreis engdoreis requested a review from rivos-eblot October 27, 2025 14:08
@rivos-eblot
Copy link

see also the PR title, as they now follow the

`component`: ...

formatting, i.e. ot_spi_device: Implement the TPM mode

nit pick: you still need to update your PR titke

@engdoreis engdoreis changed the title [ot] hw/opentitan: ot_spi_device: Implement the TPM mode ot_spi_device: Implement the TPM mode Oct 27, 2025
Based on the table in the spi_device/doc/programmers_guide

Signed-off-by: Douglas Reis <[email protected]>
@engdoreis engdoreis force-pushed the ot_spi_tpm branch 2 times, most recently from ba450b8 to c7d8e5a Compare October 27, 2025 15:37
Copy link

@rivos-eblot rivos-eblot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks, this generally LGTM after addressing my final comments. I haven't cross-referenced the implementation against the TPM spec, but a passing TPM test provides confidence.

On that note, when I checkout your OpenTitan PR and run the provided command the test does not pass, it just gets stuck after Iteration 0 and times out. Is there something I'm missing, was there a regression while making the fixes or is there something else happening? Nevermind, I didn't realize that my override path was different. It works when I override QEMU properly 😅 I'm happy after these 2 comments are addressed.

@engdoreis engdoreis merged commit 604889c into lowRISC:ot-9.2.0 Oct 29, 2025
10 checks passed
@engdoreis engdoreis deleted the ot_spi_tpm branch October 29, 2025 09:56
@rivos-eblot
Copy link

See SPI device regression introduced with this PR: #266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants