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

config_pkg: fix HPDCache related parameter #2731

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

ricted98
Copy link
Contributor

@ricted98 ricted98 commented Jan 23, 2025

The AXI AW channel in the HPDcache is shared by three components:

  • Write Buffer
  • Flush Controller
  • Uncached Controller

The ID for each transaction is generated based on its source as follows:

  • Write Buffer: {1'b0, write_buffer_entry_index}
  • Flush Controller: {1'b1, flush_controller_index}
  • Uncached Controller: '1

To distinguish between flush transactions and uncached transactions, the flush transaction ID must include at least one 0.

Currently, the AXI ID is limited to 4 bits, while the flush controller supports 8 entries. As a result, when a transaction is sent from the 8th entry of the flush controller, all bits of the ID are set to 1. This causes the HPDcache to misroute the response to the uncached controller instead of the flush controller.

The parameter CVA6ConfigWtDcacheWbufDepth is used in the WB cache to set the number of flush entries. To avoid modifying the ID width, the number of flush entries must be less than 8. Non-power-of-two values are supported.

@ricted98 ricted98 marked this pull request as ready for review January 23, 2025 14:15
Copy link
Contributor

✔️ successful run, report available here.

@JeanRochCoulon
Copy link
Contributor

@yanicasa Can you review this PR, it is close to your current activity... Thanks

@ricted98
Copy link
Contributor Author

As a side note, we could opt to set CVA6ConfigWtDcacheWbufDepth to 4 if we want to keep a power of two value.

@JeanRochCoulon
Copy link
Contributor

or (of course) @cfuguet

@cfuguet cfuguet self-assigned this Jan 23, 2025
Copy link
Contributor

@cfuguet cfuguet left a comment

Choose a reason for hiding this comment

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

Yes. This was a bug. FYI, I'm adding a specific assertion to check this parameter in the HPDcache.

@ricted98
Copy link
Contributor Author

ricted98 commented Jan 23, 2025

One comment: non power of two values will cause a port width mismatch in a decoder inside the HPDCache on the val_o port. I can also open a PR on the HPDCache to add an unused signal to fix the warning.

https://github.com/openhwgroup/cv-hpdcache/blob/04de80896981527c34fbbd35d7b1ef787a082d7c/rtl/src/hpdcache_flush.sv#L281-L285

@JeanRochCoulon
Copy link
Contributor

This PR fixes one configuration, bu what about the other configurations. Maybe the fix should be applied on all configurations?

@ricted98
Copy link
Contributor Author

ricted98 commented Jan 23, 2025

It is my understanding this issue arises with the WB HPDCache only. The parameter serves different purposes depending on the cache policy. Are there other targets with the WB HPDCache?

https://github.com/openhwgroup/cv-hpdcache/blob/04de80896981527c34fbbd35d7b1ef787a082d7c/rtl/src/hpdcache.sv#L980-L1038

@JeanRochCoulon JeanRochCoulon merged commit 024b8ea into openhwgroup:master Jan 23, 2025
4 checks passed
@JeanRochCoulon
Copy link
Contributor

Thanks !

@cfuguet
Copy link
Contributor

cfuguet commented Jan 24, 2025

Yes, I confirm that it is the only configuration concerned.

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.

3 participants