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

[topgen] Generate files for all address spaces #25894

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jan 16, 2025

Individually generate C and Rust collateral for all address spaces

Fix up alert and irq domains to enable generating collateral where the
alerts and irqs don't necessarily generate from devices in the same
address space as the CPU. Darjeeling has bus bridges that make servicing
those interrupts possible (indirect reachability to the IP).

The autogenerated tests will surely be bonkers, but that is to be fixed
up later.

@a-will a-will force-pushed the addr-space-output-redo branch from fb78b14 to 062ed30 Compare January 16, 2025 15:28
@a-will a-will changed the title WIP Start handling indirectly reachable IPs on same alert / irq domains [topgen] Generate files for all address spaces Jan 16, 2025
@a-will a-will force-pushed the addr-space-output-redo branch 2 times, most recently from a7dae8e to 6bcfc4a Compare January 16, 2025 16:36
@a-will
Copy link
Contributor Author

a-will commented Jan 16, 2025

This PR reworks #23122 a bit and adds comments that explain what the functions are trying to do and where there are limitations.

@a-will a-will force-pushed the addr-space-output-redo branch from 6bcfc4a to aaa22be Compare January 16, 2025 17:26
@a-will a-will marked this pull request as ready for review January 16, 2025 17:27
@a-will a-will requested review from msfschaffner and a team as code owners January 16, 2025 17:27
@a-will a-will requested review from cfrantz, pamaury, Razer6, matutem and qmn and removed request for a team January 16, 2025 17:27
Copy link
Member

@Razer6 Razer6 left a comment

Choose a reason for hiding this comment

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

That looks good. Thanks for improving my works. A quick question, right now some definitions are rendered in all address space. I guess that should only be rendered in the appropriate address space.

@a-will a-will force-pushed the addr-space-output-redo branch 2 times, most recently from 1116d0d to 7c16f7a Compare January 18, 2025 00:37
@a-will a-will requested review from Razer6 and pamaury January 18, 2025 00:42
Copy link
Member

@Razer6 Razer6 left a comment

Choose a reason for hiding this comment

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

I think there still some output definitions that need to be dropped by adapting the IP detection in the template files.

hw/top_darjeeling/sw/autogen/top_darjeeling_soc_dbg.h Outdated Show resolved Hide resolved
hw/top_darjeeling/sw/autogen/top_darjeeling_soc_dbg.h Outdated Show resolved Hide resolved
hw/top_darjeeling/sw/autogen/top_darjeeling_soc_dbg.h Outdated Show resolved Hide resolved
@a-will
Copy link
Contributor Author

a-will commented Jan 18, 2025

I think there still some output definitions that need to be dropped by adapting the IP detection in the template files.

Those were not actually focus areas of this PR (which targets just the base addresses, interrupts, and alerts), but I could look at handling them here too.

Edit: ...done. Just a simple application of the same patterns in this PR. A helper function might be in order, but I'll wait for the pass with multiple instances in a design before I bother.

@a-will a-will force-pushed the addr-space-output-redo branch from 7c16f7a to e4692ec Compare January 18, 2025 16:08
Individually generate C and Rust collateral for all address spaces

Fix up alert and irq domains to enable generating collateral where the
alerts and irqs don't necessarily generate from devices in the same
address space as the CPU. Darjeeling has bus bridges that make servicing
those interrupts possible (indirect reachability to the IP).

The autogenerated tests will surely be bonkers, but that is to be fixed
up later.

Co-authored-by: Robert Schilling <[email protected]>
Signed-off-by: Alexander Williams <[email protected]>
@a-will a-will force-pushed the addr-space-output-redo branch from e4692ec to 31a1a0a Compare January 18, 2025 16:12
@a-will a-will requested a review from Razer6 January 18, 2025 16:14
Copy link
Member

@Razer6 Razer6 left a comment

Choose a reason for hiding this comment

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

Thanks Alex for removing the unneeded definitions. This looks good to me!

@vogelpi vogelpi added the CI:Rerun Rerun failed CI jobs label Jan 20, 2025
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jan 20, 2025
@vogelpi vogelpi merged commit 955bab6 into lowRISC:master Jan 20, 2025
40 checks passed
@a-will a-will deleted the addr-space-output-redo branch January 20, 2025 16:32
@jwnrt
Copy link
Contributor

jwnrt commented Jan 20, 2025

FYI I think there was some kind of merge skew here with Rust linting:

$ bazel run //quality:rustfmt_fix
Target //quality:rustfmt_fix up-to-date:
  bazel-bin/quality/rustfmt_fix.bash
Error writing files: failed to resolve mod `top_darjeeling_soc_dbg`: opentitan/hw/top_darjeeling/sw/autogen/chip/top_darjeeling_soc_dbg.rs does not exist

I think the last time this went through CI was before 0791e55 was merged

@jwnrt
Copy link
Contributor

jwnrt commented Jan 20, 2025

Can we revert and try again?

It looks like the mod.rs in hw/top_darjeeling/sw/autogen/chip/mod.rs expects to find top_darjeeling_soc_dbg.rs but this file is actually in sw/host/opentitanlib/src/chip/autogen/top_darjeeling_soc_dbg.rs and not included in the mod.rs there.

@vogelpi
Copy link
Contributor

vogelpi commented Jan 20, 2025

I'll give it a try. Thanks @jwnrt for flagging.

vogelpi added a commit to vogelpi/opentitan that referenced this pull request Jan 20, 2025
This is a follow up of 955bab6
merged with lowRISC#25894.

Signed-off-by: Pirmin Vogel <[email protected]>
rsformat_dir / f"{topname}.rs",
helper=rs_helper)
# Generate Rust host-side files
rsformat_dir = src_tree_top / 'sw/host/opentitanlib/src/chip/autogen'
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this change of rsformat_dir inside the address space loop also affects the generation of the following device-side files which is wrong. I've now filed #25953 to fix this and to unblock CI again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I didn't pay close enough attention to the rust outputs, apparently. That code could be cleaned up a little more...

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't worry, it's working now :-)

vogelpi added a commit that referenced this pull request Jan 20, 2025
This is a follow up of 955bab6
merged with #25894.

Signed-off-by: Pirmin Vogel <[email protected]>
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.

5 participants