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

SerialAdapter should require minLatency > 0 #88

Open
mwachs5 opened this issue Jun 13, 2020 · 2 comments
Open

SerialAdapter should require minLatency > 0 #88

mwachs5 opened this issue Jun 13, 2020 · 2 comments

Comments

@mwachs5
Copy link

mwachs5 commented Jun 13, 2020

There is a state machine in the Serial Adapter that seems to assume that d_valid will not be asserted on the same cycle as a_valid when writing. It can't accept the ack on the same cycle that the data is being written. The state machine won't advance if this is the case, even though that is valid Tile Link behavior.

There should be a require(outer.node.minLatency > 0) e.g. here to detect this case, or the state machine should be corrected to handle it:

This is my understanding, the state machine won't accept the write ack (by asserting d_ready) unless it's in write ack state:

mem.d.ready := state.isOneOf(s_write_ack, s_read_data)

But it won't go into write-ack state unless the write is actually accepted:

when (state === s_write_data && mem.a.ready) {

But in tile link it is legal to have a_ready = d_ready.

@timsnyder
Copy link
Contributor

Thanks for filing this @mwachs5.

But in tile link it is legal to have a_ready = d_ready.

See also: "Request Response Ordering" (immediately before Fig 4.3) in TileLink Spec v1.8

For example, a designer might be tempted to implement a master interface which holds d ready
LOW while a valid is HIGH in order to delay a concurrent response message until the following
cycle. However, this represents an indefinite delay on Channel D that is not allowed by any of the
forward progress ready rules. Indeed, a TL-UL–conforming slave interface may have connected
d valid and d ready to a valid and a ready respectively. Thus, the non-conforming master
interface has introduced a deadlock.

If a master interface cannot deal with receiving a response message on the same cycle as its
request message, then it can instead put a buffer after its Channel D input. The buffer absorbs a
concurrent Channel D response message and presents d ready HIGH until it has been filled.

I've encountered a case where a deadlock does occur. I'd be happy to submit a PR that at least adds buffering in front of the TL interface if that would be helpful.

@zhemao
Copy link
Contributor

zhemao commented Jun 22, 2020

Thanks for pointing this out, Megan. We do this a lot throughout our TileLink code. I'll scan through all the existing code and see where else we need to add requires.

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