Skip to content

axi_to_detailed_mem: Avoid spurious write responses with HideStrb#383

Merged
micprog merged 2 commits intopulp-platform:masterfrom
MaxWipfli:axi_to_mem_dead_resp
Jul 24, 2025
Merged

axi_to_detailed_mem: Avoid spurious write responses with HideStrb#383
micprog merged 2 commits intopulp-platform:masterfrom
MaxWipfli:axi_to_mem_dead_resp

Conversation

@MaxWipfli
Copy link
Copy Markdown
Contributor

The dead response FIFO could previously underflow, because the surrounding code could present a valid read response (rvalid_o) based on dead_response, without considering whether dead_response was actually valid.

@thommythomaso thommythomaso added the bug Something isn't working label Jul 17, 2025
@MaxWipfli
Copy link
Copy Markdown
Contributor Author

Could you please check the CI failure? It looks to me like it is unrelated to my changes (failure is while installing Bender). Otherwise, please let me know if you think it's due to my changes.

@micprog
Copy link
Copy Markdown
Member

micprog commented Jul 24, 2025

I'm not sure this PR fixes a symptom or the cause of the actual issue. Could you have a look at #389 to see if that helps fix your issue?

@MaxWipfli
Copy link
Copy Markdown
Contributor Author

MaxWipfli commented Jul 24, 2025

I am not sure if #389 fixes my issue. Also, I have unfortunately not saved that state of the design, so I cannot reproduce the issue anymore.

My issue was specific to the case where there is only a single bank, and the full strobe is zero. I know this is kind of weird, but this is what was received in my case (for some reason).
In that case, resp_valid and dead_response are both single-bit signals. If dead_response is 1, rvalid_o can remain high even if there is no request currently in flight. (This cannot happen unless the full strobe is zero, which make this such an edge case.)

I am not well-versed enough in your codebase to quickly write a minimum failing example.
I think it should be reproducible by inputting a large number of writes with a fully zero strobe, and then stop. You should observe spurious B responses in case.

Looking at it differently: How do you ensure that there are never more responses (on mem_stream_to_banks_detailed::rvalid_o) than there are requests (on mem_stream_to_banks_detailed::req_i)?

I don't think #389 actually fixes that issue, but is a different (unrelated) issue. As far as I can tell, the issue fixed by #389 can only occur if there is back-pressure from the memory bank, which was not the case for me (I tied mem_gnt_i high).

@micprog
Copy link
Copy Markdown
Member

micprog commented Jul 24, 2025

Ah, I see your point. Thanks for the additional explanation!

@micprog micprog merged commit 6957453 into pulp-platform:master Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants