-
Notifications
You must be signed in to change notification settings - Fork 7
Bump Snitch cluster to latest revision #81
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
base: devel
Are you sure you want to change the base?
Conversation
Lore0599
left a comment
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.
Thanks a lot for the valuable work.
Overall LGTM, I left some small comments.
| // ------------ | ||
| // | TCDM | | ||
| // ------------ | ||
| localparam doub_bt TcdmSize = 128; | ||
| localparam aw_bt TcdmAddrWidth = $clog2(TcdmSize * 1024); | ||
|
|
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.
These parameters are not related to the SoC. but are mainly related to the cluster. Maybe it might make sense to move them somewhere else?
Also we are not using the external TCDM ports, so I think that actually those parameters can be removed in general and when instantiating the Snitch cluster we don't specify them and use the default types.
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.
In general, since we are not using the new ports, we can avoid generating intermediate types. If we do not specify the type at the interface, the default types will be used. Then we can simply leave the port unconnected, as you already did.
What do you think?
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.
Yeah, I agree it would be cleaner the way you suggest. However, I've tried not defining the tcdm_dma_*_t types, and it can't compile. The default type for them in snitch_cluster module is logic, which doesn't seem to be supported by snitch_tcdm_interconnect and other modules down the hierarchy.
I don't think we can avoid defining tcdm_dma_*_t. In that case, since it's cluster-specific, probably it's better to move the definition of TcdmSize and TcdmAddrWidth inside chimera_cluster, right?
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.
okay, then it makes sense to open an issue on Snitch.
As a temporary fix to merge this PR the parameters can be moved inside the chimera_cluster to make sure that it is cluster specific
6e47585 to
ff4a899
Compare
This PR updates the Snitch cluster to pulp-platform/snitch_cluster@5b2fccd.
Main changes
axito https://github.com/colluca/axi (branchmulticast).chimera_pkgto match the current Snitch cluster's defaults.chimera_cluster, adding connections to new Snitch cluster's ports.Verification
sw/testspass in vsim.