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

[sram_ctrl] Consider refactoring tlul_adapter_sram #7462

Open
msfschaffner opened this issue Jul 24, 2021 · 4 comments
Open

[sram_ctrl] Consider refactoring tlul_adapter_sram #7462

msfschaffner opened this issue Jul 24, 2021 · 4 comments
Assignees
Labels
Component:RTL Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones IP:sram_ctrl Priority:P3 Priority: low Type:Cleanup Cleanup tasks Type:FutureRelease Not relevant to currently planned releases/milestones
Milestone

Comments

@msfschaffner
Copy link
Contributor

msfschaffner commented Jul 24, 2021

This module has grown quite a bit over time and seems a bit difficult to read and debug by now.
Also, the byte write logic inside tlul_sram_byte needs additional request FIFOs which could potentially be consolidated with the rest of the tlul_adapter_sram FIFOs if we were to refactor this module.

In addition to the above, also consider creating different wrapper versions of the module to simplify the parameter usage.
Currently there are too many parameters controlling whether integrity is checked, generated or passed through. This makes usage of tlul_adapter_sram error prone, as was observed in #9406

@tjaychen
Copy link

i actually specifically chose to separate those instead of combining them.
If we need to insert stalls in the adapter sram protocol, it actually creates a lot of complexity on the existing logic handling.

it seems cleaner to separate them by function.

@msfschaffner
Copy link
Contributor Author

ok, I see. the reason why I brought up the consolidation above is that I had to add another FIFO to keep the a_size inside tlul_sram_byte in #7218, which makes it somewhat redundant with the request FIFO in tlul_adapter_sram (this was needed to return the correct a_size in case of byte writes, since the tl_sram_i write response is currently always indicating full word writes due to the read modify write operation). I was therefore thinking that maybe we can move the gasket to the SRAM side such that this additional req FIFO is not necessary anymore.

this is not critical however, and more meant as a cleanup task if we have time.

@tjaychen
Copy link

yeah let's think about it... so originally i split it out because the FIFO's inside tlul_adapter_sram deposited an entry once it was accepted by sram. The problem with the sram read modified write is that I need to accept the entry, but also change the back pressuring scheme such that it will deposit the entry even if sram side had not accepted the transaction (because we're are still in the middle of writing the last one).

Thinking about it more, it probably does make sense to see how we can merge this better. I think mainly the tlul->sram handling will need to be tweaked a bit to de-couple acceptance of sram transaction and tlul transaction.

@tjaychen tjaychen added IP:sram_ctrl Priority:P2 Priority: medium labels Mar 3, 2022
@tjaychen tjaychen added the Type:FutureRelease Not relevant to currently planned releases/milestones label May 11, 2022
@msfschaffner msfschaffner added Priority:P3 Priority: low and removed Priority:P2 Priority: medium labels Aug 24, 2022
@msfschaffner msfschaffner added this to the Backlog milestone Oct 28, 2022
@andreaskurth
Copy link
Contributor

Triaged for sram_ctrl, keeping Type:FutureRelease Not relevant to currently planned releases/milestones as potential RTL refactoring in a future release.

@msfschaffner msfschaffner added the Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 6, 2023
This was referenced Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones IP:sram_ctrl Priority:P3 Priority: low Type:Cleanup Cleanup tasks Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

No branches or pull requests

6 participants