-
Notifications
You must be signed in to change notification settings - Fork 678
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
CHANGE: Organize abstract config into sections and change comment format to docstrings #1722
Conversation
@jerryz123 I'm not sure if I put the all fragments into correct sections. |
It might be easier to converge if we agree on a outline of the organization, and then modify the Abstract config. A proposal:
|
I think this outline looks good in general. However, i want to put everything that is on the memory map to the same section, basically putting the "On-chip memory", "Off-chip memory", and "MMIO Devices" together. |
and clock/reset section will also include power (if any), right? |
I feel like this is hard to accomplish, since a bunch of random things will alter the memory map
Yes, but we don't have anything for that right now |
Could we do this instead?:
This way, the config fragment corresponds the blocks in the SoC block diagram. Sections still subject to change. I think we can try and see if it works, and if not, we can always make changes later. |
That organization looks good to me. |
@T-K-233 I made some minor changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
// ================================================ | ||
// Set up Tiles | ||
// ================================================ | ||
// tile-local settings goes here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this section if it's empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a placeholder. The idea is that ChipLikeRocket config will also use this template, and configs like WithFPU will go here.
@T-K-233 can you rebase then address Abe's comments |
Fixed |
Related PRs / Issues:
Type of change:
Impact:
Contributor Checklist:
main
as the base branch?changelog:<topic>
label?changelog:
label?.conda-lock.yml
file if you updated the conda requirements file?Please Backport
?