atop_resolver: Fix handshaking and other bugs#34
Open
LucaRufer wants to merge 1 commit intopulp-platform:mainfrom
Open
atop_resolver: Fix handshaking and other bugs#34LucaRufer wants to merge 1 commit intopulp-platform:mainfrom
LucaRufer wants to merge 1 commit intopulp-platform:mainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current implementation of the
atop_resolverhas issues with handshake signal dependencies and relying on the module connected to its master port to always respond in the next cycle. Both of those issues violate the OBI specification and can cause the module to deadlock. These issues and other smaller fixes are addressed in this PR.Major Issues fixed:
RegisterAmoparameter is set to 0, the write-back request of the AMO is performed in the same cycle as the AMO load. This causesmgr_port_req_o.reqto depend onmgr_port_rsp_i.rvalid, which violates R-21.1 of the OBI spec (v1.6.0): "For a manager, req shall not combinatorially depend on gnt or rvalid". The parameter was changed to default to1for OBI compliance and a comment was added about the non-compliance if the parameter is set to0.mgr_port_req_o.a.wedepended onmgr_port_rsp_i.gnt, violating the OBI spec.DoAMOandWriteBackAMOstates locked up ifmgr_port_rsp_i.gntwas not asserted in the same cycle as the response handshake.RegisterAmowas set to1, the state machine locked up on any AMO operation.last_amo_wbused to prevent forwarding the response of the AMO write-back to the sbr port (as the sbr port gets the response of the AMO load) relied on the response arriving exactly one cycle after the request. If the response arrived later, it was falsely forwarded to the master port. This issue was fixed by replacing it with theamo_ignore_rsp_qsignal, which is set when the AMO write-back is issued and cleared on the next response handshake of the mgr port (Which is guaranteed to correspond to the AMO write-back as it is the only outstanding one).sc_qandsc_successful_or_lr_qsignal relied on the response arriving in the next cycle. This issue was fixed by adding the signals to the metadata fifo to make them available once the response arrives.Minor Fixes:
NumTxns(added in atop_resolver: Add support for outstanding transactions #24) to2for full throughput (1 per cycle) of non-AMO transfers. Otherwise, thegntis retracted after a handshake due to the credit counter and only asserted inthe cycle after the rvalid.rdata_usagesignal.exokayfromrdatabuffer, as it is already determined by the LR/SC when the request arrives. It was moved to the metadata queue.out_rdataoverwrite for SC if the response does not arrive after one cycle.rsp_happeningifMgrPortObiCfg.UseRReadyenabled.sbr_port_rsp_o.r.r_optional.ruserusing wrong data.In summary, the module was refactored to ensure the following: