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

Use "tile" instead of "core" to assign freq's in WithTileFrequency config. fragment #807

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

abejgonzalez
Copy link
Contributor

Related issue:

Type of change: bug fix

Impact: other

Release Notes
Fixes an issue where WithTileFrequency wouldn't modify the frequency of all tiles since clock names are based on the TileParams name (which defaults to tile and boom_tile for Rocket/BOOM respectively).

Tested with 2 configs:

  1. MulticlockRocketConfig:
    image

  2. DoubleRocketAndBoomConfig (custom config that only used WithTileFrequency(600) - no other freq's changed):
    image

@abejgonzalez abejgonzalez self-assigned this Feb 26, 2021
@abejgonzalez abejgonzalez changed the title Use "tile" instead of "core" to assign freq's Use "tile" instead of "core" to assign freq's in WithTileFrequency config. fragment Feb 26, 2021
@jerryz123
Copy link
Contributor

I remember this was done because it defaults naming everything to core_{hartid}, done here: https://github.com/chipsalliance/rocket-chip/blob/8f9149faf01fec9725d08c31c9c0bb5341f5e999/src/main/scala/subsystem/HasTiles.scala#L251

Was there a change somewhere that changed the naming to tile?

@abejgonzalez
Copy link
Contributor Author

I saw that... The getOrElse is getting what <Rocket/Boom>TileParams gives: https://github.com/chipsalliance/rocket-chip/blob/8f9149faf01fec9725d08c31c9c0bb5341f5e999/src/main/scala/tile/RocketTile.scala#L25. I'm not sure how this worked before...

@abejgonzalez abejgonzalez requested a review from jerryz123 March 1, 2021 18:38
Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Approved, but @davidbiancolin should take a look as well

@davidbiancolin
Copy link
Contributor

davidbiancolin commented Mar 3, 2021

It was named core before (yay strings). This LGTM.

@abejgonzalez abejgonzalez merged commit a165763 into dev Mar 3, 2021
@abejgonzalez abejgonzalez deleted the tile_clk_assignment_fix branch May 3, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants