Skip to content
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.

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

[OpenTitan-tool] Bootstrapping should trigger a POR to ensure state is uniform at the start of tests #13098

Closed
drewmacrae opened this issue Jun 7, 2022 · 13 comments · Fixed by #13277
Assignees
Labels
Priority:P1 Priority: high Type:Cleanup Cleanup tasks Type:Question Questions

Comments

@drewmacrae
Copy link
Contributor

After bootstrapping if opentitan-tool triggers a POR, tests will always start with just the POR bit set. This allows for simple tests that use the reset_info to determine that they're running the test software for the first time, and could resolve #13096

@a-will
Copy link
Contributor

a-will commented Jun 7, 2022

After bootstrapping if opentitan-tool triggers a POR, tests will always start with just the POR bit set. This allows for simple tests that use the reset_info to determine that they're running the test software for the first time, and could resolve #13096

To be clear, it only helps for the tests as they are run today. On top of the mask ROM, they would fail, since the mask ROM moves the reset reasons to the retention SRAM. 😄

Avoiding a test start with the software reset bit set allows tests that exercise software resets to freely do what they need.

@a-will
Copy link
Contributor

a-will commented Jun 10, 2022

The answer from the chip-level test meeting was "yes"

@drewmacrae drewmacrae added Type:Cleanup Cleanup tasks and removed Type:Question Questions labels Jun 11, 2022
@drewmacrae drewmacrae changed the title [OpenTitan-tool] Should bootstrapping trigger a POR to ensure state is uniform at the start of tests [OpenTitan-tool] Bootstrapping should trigger a POR to ensure state is uniform at the start of tests Jun 11, 2022
@drewmacrae drewmacrae added the Type:Question Questions label Jun 15, 2022
@drewmacrae
Copy link
Contributor Author

I'm fairly certain it's currently mapped to a physical switch that we don't have control over. Should we remap it? Who should we ask about allocating an FPGA pin to this?

@a-will
Copy link
Contributor

a-will commented Jun 15, 2022

I'm fairly certain it's currently mapped to a physical switch that we don't have control over. Should we remap it? Who should we ask about allocating an FPGA pin to this?

If it is only mapped to the button right now, I'll take care of adding the GPIO to SAM3X.

@alphan
Copy link
Contributor

alphan commented Jun 15, 2022

I'm not sure if we really need this. Is there a reason why tests can't just check if the POR is set, instead of requiring that only the POR bit is set? Once they start, they clear the reset reasons anyway.

@a-will
Copy link
Contributor

a-will commented Jun 15, 2022

I'm not sure if we really need this. Is there a reason why tests can't just check if the POR is set, instead of requiring that only the POR bit is set? Once they start, they clear the reset reasons anyway.

Because the other reset reasons are reserved for the test to modify, and we do not require bootstrapping to occur as part of the test methodology across all platforms.

Issuing a POR is an extremely simple thing to do, and this expectation has already been baked in to tests. Let's just get it done.

@alphan
Copy link
Contributor

alphan commented Jun 15, 2022

Issuing a POR is an extremely simple thing to do, and this expectation has already been baked in to tests. Let's just get it done.

SGTM but it would be nice to make this optional to be able to verify a bootstrap sequence that ends with RESET.

@a-will
Copy link
Contributor

a-will commented Jun 15, 2022

Issuing a POR is an extremely simple thing to do, and this expectation has already been baked in to tests. Let's just get it done.

SGTM but it would be nice to make this optional to be able to verify a bootstrap sequence that ends with RESET.

Totally agreed 😄

It would merely become associated with a specific test now and be part of its stimulus. (Aside: similarly, we'll also want something for spi_device generic mode, since the bootstrapping changes now have caused it to lose coverage.)

@drewmacrae
Copy link
Contributor Author

@a-will
Copy link
Contributor

a-will commented Jun 15, 2022

I think there are three unused pins here: https://cs.opensource.google/opentitan/opentitan/+/master:hw/top_earlgrey/data/pins_cw310.xdc;drc=a35f7687666385502591fafed977ab83475d19b6;l=71

For the pin, we'll likely be using whatever is the highest unused USB_A* pin, possibly with inverted polarity (so the default is to not be in reset). The section you've got there is for a header, which will be useful for the hyperdebug version later.

Edit: Right... pins probably start high-impedance. So never mind on the inverted polarity part: We'll just attach a pull-up property on the FPGA side. But the principle is the same: Default to not being in reset.

@a-will
Copy link
Contributor

a-will commented Jun 20, 2022

It turns out the POR is already hooked up on CW310 via USB_A14. This pin was pretending to be a JTAG TAP reset, but because it is also used as a power-on reset, we're actually lacking a TAP reset pin. That cleanup will happen with the rest of the JTAG pins, several of which currently are multiplexed on top of the SPI pins.

@alphan
Copy link
Contributor

alphan commented Jun 21, 2022

#13277 should resolve this as a side effect.

@a-will a-will linked a pull request Jun 24, 2022 that will close this issue
@a-will
Copy link
Contributor

a-will commented Jun 24, 2022

Resolved by #13277. Thanks, @alphan!

matutem added a commit to matutem/opentitan that referenced this issue Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:P1 Priority: high Type:Cleanup Cleanup tasks Type:Question Questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants