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

SECDED errors from the SRAM can not be signalled for ReadModifyWrite operations. #179

Open
ColinDBates opened this issue Sep 3, 2024 · 2 comments

Comments

@ColinDBates
Copy link

As stated in sonata-system/vendor/lowrisc_ip/ip/tlul/rtl/tlul_adapter_sram.sv the SRAM adapter does not wait for a response from the SRAM before sending able the d_valid for write operations, see lines 474-477:

  //    interleaved. So, to make it in-order (even TL-UL allows out-of-order
  //    responses), storing the request is necessary. And if the read entry
  //    is write op, it is safe to return the response right away. If it is
  //    read reqeust, then D response is waiting until read data arrives.

But for SRAM's with RMW, required for byte writing to a ECC protected SRAM for instance, the SRAM can signal a Double-Error-Dectect error on a write operation.

The current implementation, does not wait for the SRAM to return rvalid_i/rerror_i on write operations before sending the d_valid on the TL interface, so any SECDED error on a RMW operation will be lost.

My suggestion is to require rvalid_i assertion before sending d_valid on write operations.

@marnovandermaas
Copy link
Contributor

This may be related to a previous issue I have seen: #28

I agree this needs investigation and fixing.

@GregAC
Copy link
Contributor

GregAC commented Sep 4, 2024

Ultimately we don't aim to support ECC on Sonata so there's an argument for keeping things as simple as possible and not supporting the RMW behaviour in the Sonata SRAM wrapper. Though should be an easy fix so probably worth doing.

I do have a simple testbench I did for the hyperram controller that does random read/write tests over tilelink checking responses against a memory model (which will be making its way into the repository once I've cleaned it up). We could also run that against the SRAM wrapper.

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

No branches or pull requests

3 participants