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

Add sram update by a sram read #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Aquaticfuller
Copy link

Add sram update when a sram read finds a correctable error and corrects it.

@micprog micprog self-requested a review May 3, 2024 13:28
@micprog
Copy link
Member

micprog commented May 3, 2024

Hi @Aquaticfuller,
Could you rebase these changes on the new master branch I just pushed? It incorporates significant improvements to the ecc_sram in a new module, so I think it would make most sense to focus the changes there, as it is significantly more flexible.

Regarding the specific changes, I have some notes:

  • I think it would be better if the correction of a load with a single error does not stall a subsequent access and only gets corrected if the cycle is available. This way there is less performance impact of the change. With the general assumption that errors are unlikely, subsequent accesses with another error should not happen (but of course need to be considered and properly handled, but I think dropping one correction if needed should be OK behavior). What do you think?
  • Ideally, this change can be enabled and disabled with a parameter. Do you see a way to add one?
  • If we go with the change listed above, where a correction in a subsequent cycle is not guaranteed, it would make sense to have a separate reporting signal for this correction, signalling that the single-error access was corrected, to ensure the correction is properly logged. Currently, we can just use the single-error register for this.

@Aquaticfuller
Copy link
Author

Hi @micprog,
Of course, it makes sense. Just one concern, given that now in the LLC, the ecc_sram_wrappers are using an outside scrubber, and to simplify the interface, the outside scrubber shares the same read interface as common read requests, so the outside scrubber error correction also relies on this self-correction method. This less stall design would make the outside scrubber correction also have a very low possibility of being dropped; I estimate this won't cause any extra error unless one more error accumulates on that block, but this should be fine with a very low possibility.

@micprog
Copy link
Member

micprog commented May 3, 2024

Hi @micprog, Of course, it makes sense. Just one concern, given that now in the LLC, the ecc_sram_wrappers are using an outside scrubber, and to simplify the interface, the outside scrubber shares the same read interface as common read requests, so the outside scrubber error correction also relies on this self-correction method. This less stall design would make the outside scrubber correction also have a very low possibility of being dropped; I estimate this won't cause any extra error unless one more error accumulates on that block, but this should be fine with a very low possibility.

I agree, it should work out, and the scrubber will come back around to this address in case it is dropped. Maybe you could implement a parameter such that this behavior is configurable? Then you can guarantee correction or have slightly better performance, depending on how you want your design to work.

@Aquaticfuller
Copy link
Author

Hi @micprog, Of course, it makes sense. Just one concern, given that now in the LLC, the ecc_sram_wrappers are using an outside scrubber, and to simplify the interface, the outside scrubber shares the same read interface as common read requests, so the outside scrubber error correction also relies on this self-correction method. This less stall design would make the outside scrubber correction also have a very low possibility of being dropped; I estimate this won't cause any extra error unless one more error accumulates on that block, but this should be fine with a very low possibility.

I agree, it should work out, and the scrubber will come back around to this address in case it is dropped. Maybe you could implement a parameter such that this behavior is configurable? Then you can guarantee correction or have slightly better performance, depending on how you want your design to work.

Yes, that makes a lot of sense.

@micprog micprog force-pushed the zx/corrected_data_sram_update branch from 6d2b25f to 0d7b91b Compare July 2, 2024 16:21
@micprog micprog force-pushed the zx/corrected_data_sram_update branch from 0d7b91b to 1835197 Compare July 2, 2024 16:24
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.

2 participants