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

[bazel] unify cw310_params, cw310/hyper310_jtag_params #21645

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

nbdd0121
Copy link
Contributor

The functionality is integrated to cw310_params as extra parameters. The intention is that one can choose to enable JTAG without having to clear the bitstream.

For backward compatibility of the rules, *_jtag_params are set to clear the bitstream.

If test_harness is specified, then --bitstream={bitstream} is automatically added to test_cmd.

@nbdd0121 nbdd0121 requested a review from pamaury February 23, 2024 12:40
@nbdd0121 nbdd0121 changed the title [bazel] unifty cw310_params, cw310/hyper310_jtag_params [bazel] unify cw310_params, cw310/hyper310_jtag_params Feb 23, 2024
Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

This looks good to me, I think it's a good thing to unify those parameters.

Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be nice if need_jtag were a Boolean and opentitan_test added the correct OpenOCD flags based on the exec_env.param.interface. Do you think that would be possible?

The advantage is it lets us share the same cw310_params between hyper310 and cw310 execution environments. These are basically always exactly the same, and possibly should just be merged in the future.

if not bitstream:
bitstream = "@//hw/bitstream/universal:splice"
return struct(
tags = ["hyper310", "exclusive"] + tags,
Copy link
Contributor

Choose a reason for hiding this comment

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

The interface-specific tags have been dropped here. We seem to have moved over to using a tag derived from the Target's label, though, so maybe we can just eliminate the interface tag for cw310 as well.

We'll probably need to overhaul test filtering for multi-top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I think the current "hyper310", "cw310" label isn't accurate though because there's no hyper310_params so anything not using jtag is still tagged with cw310. I'll see if any of our CI relies on the interface tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point but I agree with @nbdd0121 that those labels are not accurate, I think the exec_env labels are more reliable.

Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

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

Thanks for making that change, the updates look good.

Nit: s/need_jtag/needs_jtag/g

This commit unifies cw310_params and cw310/hyper310_jtag_params by
integrating JTAG support to cw310_params as extra parameters.
The OpenOCD adapter config file (if any) is moved to execution
environments.

The intention is that one can choose to enable JTAG without having
to clear the bitstream. For backward compatibility of the rules,
*_jtag_params are set to clear the bitstream.

If test_harness is specified, then `--bitstream={bitstream}` is automatically
added to test_cmd. This was the behaviour for cw310_jtag_params, but is
generally useful for all other tests.

Co-authored-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
This commit removes all usage of *_jtag_params and convert them to use
of `needs_jtag` and `changes_otp` (if necessary).

Signed-off-by: Gary Guo <gary.guo@lowrisc.org>
@pamaury pamaury merged commit ec74233 into lowRISC:master Feb 29, 2024
30 of 32 checks passed
@nbdd0121 nbdd0121 added the CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival label Feb 29, 2024
Copy link

Backport failed for earlgrey_es_sival, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_es_sival
git worktree add -d .worktree/backport-21645-to-earlgrey_es_sival origin/earlgrey_es_sival
cd .worktree/backport-21645-to-earlgrey_es_sival
git switch --create backport-21645-to-earlgrey_es_sival
git cherry-pick -x 830c4ba034a7958ef97c063e8f1eda5f85f4c1e4 181abdd4a82cd23baca00e079846b4369942339f

@nbdd0121
Copy link
Contributor Author

Backporting depends on #21580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants