Skip to content

atop_resolver: Fix handshaking and other bugs#34

Open
LucaRufer wants to merge 1 commit intopulp-platform:mainfrom
mosaic-soc:pr/atop-resolver-fix
Open

atop_resolver: Fix handshaking and other bugs#34
LucaRufer wants to merge 1 commit intopulp-platform:mainfrom
mosaic-soc:pr/atop-resolver-fix

Conversation

@LucaRufer
Copy link

The current implementation of the atop_resolver has 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:

  • When the RegisterAmo parameter is set to 0, the write-back request of the AMO is performed in the same cycle as the AMO load. This causes mgr_port_req_o.req to depend on mgr_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 to 1 for OBI compliance and a comment was added about the non-compliance if the parameter is set to 0.
  • Refactor AMO state machine. The implementation had multiple issues:
    • mgr_port_req_o.a.we depended on mgr_port_rsp_i.gnt, violating the OBI spec.
    • The DoAMO and WriteBackAMO states locked up if mgr_port_rsp_i.gnt was not asserted in the same cycle as the response handshake.
    • When RegisterAmo was set to 1, the state machine locked up on any AMO operation.
    • The last_amo_wb used 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 the amo_ignore_rsp_q signal, 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).
  • Refactor the LR/SC state storage. The sc_q and sc_successful_or_lr_q signal 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:

  • Default parameter NumTxns (added in atop_resolver: Add support for outstanding transactions #24) to 2 for full throughput (1 per cycle) of non-AMO transfers. Otherwise, the gnt is retracted after a handshake due to the credit counter and only asserted inthe cycle after the rvalid.
  • Removed unused rdata_usage signal.
  • Removed exokay from rdata buffer, as it is already determined by the LR/SC when the request arrives. It was moved to the metadata queue.
  • Fix out_rdata overwrite for SC if the response does not arrive after one cycle.
  • Fix rsp_happening if MgrPortObiCfg.UseRReady enabled.
  • Fix sbr_port_rsp_o.r.r_optional.ruser using wrong data.

In summary, the module was refactored to ensure the following:

  • The state changes and FIFO push/pops only happen based on handshake signals.
  • Dependencies between OBI signals resolved according to OBI v1.6.0
  • Minot bug fixes

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.

1 participant