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 "ready" signal to incoming data #664

Closed
tomeroberts opened this issue Mar 9, 2020 · 6 comments
Closed

Add "ready" signal to incoming data #664

tomeroberts opened this issue Mar 9, 2020 · 6 comments
Assignees
Labels
Type:Enhancement Feature requests, enhancements

Comments

@tomeroberts
Copy link

tomeroberts commented Mar 9, 2020

As part of the icache development work, it was proposed to add a ready signal from the core to stall incoming data on instr_rdata_i. Creating this issue to gather thoughts on whether this would be a good idea.

Upsides

Downsides

  • Changes pin compatibility at the top level.
  • Would be annoying / painful to add as a parameterized option.
  • As noted by @rswarbrick back-pressure could have an adverse effect on system performance where there is system bus contention.

Compatibility

  • Easily compatible with TL-UL interfaces, which already have that signal.
  • Easy enough for AXI which already has similar signalling.
  • Should be doable with AHB.
  • Wishbone I think should support this with it's retry signaling.
@tomeroberts tomeroberts added the Type:Enhancement Feature requests, enhancements label Mar 9, 2020
@tomeroberts tomeroberts self-assigned this Mar 9, 2020
@imphil
Copy link
Contributor

imphil commented Mar 13, 2020

Sounds reasonable to me. How does that relate to the concerns others had about our bus interface?
@davideschiavone for the wider picture on the other implementations using this style of bus
@eunchan for TL-UL

@GregAC
Copy link
Collaborator

GregAC commented Mar 13, 2020

I would note there's a credit based flow control discussion on the OT repo: lowRISC/opentitan#1644

Credit based could be nicer but again makes the interface more complex.

@imphil
Copy link
Contributor

imphil commented Apr 1, 2020

@Silabs-ArjanB just posted a doc to a better specified/revised version of the bus standard we use at https://github.com/openhwgroup/core-v-docs/blob/master/cores/cv32e40p/OBI-v1.0.pdf, and it contains a rready signal. That's what you're looking for here as well, @tomroberts-lowrisc?

@Silabs-ArjanB
Copy link

Silabs-ArjanB commented Apr 1, 2020

To me rready seems a logical addition (and can indeed be used to safe buffering space). If not implemented (as on RI5CY/CV32E40P) then its value is assumed 1'b1 always. Of course if a CPU uses such an rready signal, then it puts a not-backward compatible requirement on its environment (and therefore it would be nice if such addition would be parameterized).

@eunchan
Copy link

eunchan commented Apr 1, 2020

I always prefer sending request only when the requester (TL master) has enough space to accept the request to sending predictably. It needs more space in case of DMA engine But it prevents any deadlock issue. I'd rather have bigger core area to be safe.

@GregAC
Copy link
Collaborator

GregAC commented Dec 10, 2021

There are some wider questions around Ibex's memory interfacing to resolve, this can be considered as part of them. However for the foreseeable future the top-level interface will remain as it is.

@GregAC GregAC closed this as completed Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Enhancement Feature requests, enhancements
Projects
None yet
Development

No branches or pull requests

5 participants