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

Add test for Mailbox JTAG accesses with clock gating #197

Merged

Conversation

robertszczepanski
Copy link
Contributor

Related to #162.
This PR adds a test for JTAG accesses while clock gating is enabled and the core is halted.

@robertszczepanski
Copy link
Contributor Author

robertszczepanski commented Aug 29, 2023

After recent merges to caliptra-rtl, forcing security_state.debug_locked state doesn't seem to affect cptra_in_debug_scan_mode in clock gate module which results in all clocks being enabled.

@bharatpillilli @upadhyayulakiran can you confirm that the behaviour described above is expected?

However, before these changes, when testing at 8bb19ac I was able to test JTAG accesses while clocks were gated and there was one issue with mailbox read pointer. The soc_ifc_clk_cg clock was enabled for a single cycle (on JTAG access) which was not enough to both set mbox_rdptr_nxt and propagate it to mbox_rdptr. Extending clock enable to 2 cycles on each access seemed to resolve that issue.

FYI @tmichalak @mkurc-ant

@upadhyayulakiran
Copy link
Contributor

@robertszczepanski, thank you for testing this. For the first scenario, cptra_in_debug_scan_mode will only be captured upon exiting reset. So, if security_state.debug_locked was 1 at that time, we won't see a toggle on cptra_in_debug_scan_mode. That said, expected behavior is clks should be enabled if debug mode is unlocked. If debug mode is locked, clks will be enabled based on other activities. Are you unlocking debug mode after reset in your test? Could you try doing it before the reset?

For the second case, could you share your waves? Clks should be awake for as long as dmi_reg_en is active. Is dmi_reg_en 1 only for 1 clk in your test? I would like to confirm the behavior in the waves so I can also update my local test

@robertszczepanski
Copy link
Contributor Author

robertszczepanski commented Aug 30, 2023

@upadhyayulakiran we are building Caliptra with DEBUG_UNLOCKED=1 flag which sets security_state.debug_locked=0 and is mandatory in order to use JTAG interface. Having said that, it doesn't seem to be possible to keep security_state.debug_locked=1 upon exiting reset to enable clock gating because it will disable JTAG access. Is there any way to achieve both JTAG access and clock gating simultaneously so we can reproduce scenario described in #162?

Regarding dmi_reg_en, I can see that it's asserted for 1 cycle only. I am attaching a screenshot and waveforms so you can see it yourself.
caliptra_unlocked_waveforms.tar.gz
dmi_reg_en

@bharatpillilli
Copy link
Collaborator

Can we schedule a quick call and closer this off since we want to mark 1p0 soon

@upadhyayulakiran
Copy link
Contributor

@robertszczepanski @bharatpillilli
If debug_mode.locked = 0 is a requirement for JTAG accesses, we don't have a problem then right? Clocks will be enabled as long as we're in debug mode, so the read pointer should be calculated correctly. I filed #162 only by inspection assuming JTAG access can happen outside of debug mode. But now I understand that's not the case. As long as clks are enabled for the entire time JTAG is active, we should be fine. I can also see, from the waveform you shared, that clks are not turning off which is expected.

Please let me know if I'm missing something

@tmichalak
Copy link
Contributor

@upadhyayulakiran this is exactly what we see and what is in the waveforms, i.e. when debug_mode.locked = 0 all clocks are there and we can read the mailbox, if debug_mode.locked = 1 the clocks are gated, but we cannot access via JTAG. If cptra_in_debug_scan_mode will only be captured upon exiting reset (and we build caliptra with security_state.debug_locked=0) then re-enabling the clock gating is not possible. Therefore we don't see an option to have both clock gating and JTAG access. If the described behavior is intended then the test confirms it.

@upadhyayulakiran
Copy link
Contributor

@tmichalak, @robertszczepanski I think we're on the same page now. You can close the original issue (#162) and merge your PR. Thanks!

@tmichalak
Copy link
Contributor

@upadhyayulakiran thanks, however we need a review in order to get this merged. @bharatpillilli should we open this PR targeting a different branch or is main ok?

@calebofearth
Copy link
Collaborator

@upadhyayulakiran thanks, however we need a review in order to get this merged. @bharatpillilli should we open this PR targeting a different branch or is main ok?

Tomasz, please retarget this PR towards dev-public? We'll merge dev-public towards dev-integrate (then main) in the next repo sync.

@kgugala
Copy link
Member

kgugala commented Oct 3, 2023

@calebofearth dev-public seems to be pretty old (it is 173 commits behind main) shouldn't we first bump dev-public to be inline with main?

@calebofearth
Copy link
Collaborator

@calebofearth dev-public seems to be pretty old (it is 173 commits behind main) shouldn't we first bump dev-public to be inline with main?

Good thought. #238

@kgugala kgugala changed the base branch from main to dev-public October 4, 2023 05:54
@kgugala
Copy link
Member

kgugala commented Oct 4, 2023

I retargeted the PR to dev-public

@andreslagarcavilla andreslagarcavilla merged commit 5eb4852 into chipsalliance:dev-public Oct 25, 2023
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.

7 participants