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

Question about multi-port version #1

Open
edanuff opened this issue Oct 25, 2023 · 5 comments
Open

Question about multi-port version #1

edanuff opened this issue Oct 25, 2023 · 5 comments

Comments

@edanuff
Copy link

edanuff commented Oct 25, 2023

This looks really good but it looks like a lot of work does done to make it almost multi-port. Any guidance on what more would need to be done for it to be multi-port and if there were any particular issues that prevented that part from being completed? I need to use this in a multi-port situation so I'm looking at adding that capability.

@agg23
Copy link
Owner

agg23 commented Oct 25, 2023

I never ended up needing it in a multi-port scenario, so I never finished the implementation. If you only need ports of the same size and only one burst type (off/burst length), I believe the only implementation work you'd need to do is choosing how to schedule the ports (priority order, round robin, etc). If you wanted to have each port have it's own burst config, you would have to keep track of the current SDRAM setting, and change it as necessary when servicing reads/writes.

If you do the work to add the functionality, feel free to PR it. It may be a while before I'm able to review, but I'd like to get that functionality in there.

@edanuff
Copy link
Author

edanuff commented Oct 25, 2023

Thanks for the quick response. I built a multi-port arbiter that sits on top of the current sdram controller I'm using but it would be preferable do it in one place. I'll take a look at adding it and send a PR if I make progress on it.

@edanuff edanuff closed this as completed Oct 25, 2023
@edanuff
Copy link
Author

edanuff commented Oct 29, 2023

I have what looks like working code for this here. I'm still testing but it appears to work in simulation and actual hardware. I'll send a pull request once I'm more sure it's working.

@edanuff edanuff reopened this Oct 29, 2023
@agg23
Copy link
Owner

agg23 commented Oct 30, 2023

At a glance it looks nice; I like how you combined everything into one port so it's generic and can be generated. I look forward to the PR (no rush).

@edanuff
Copy link
Author

edanuff commented Oct 30, 2023

I did make a few changes based on what I saw in the simulation beyond the multi-port support. It seemed you might have had some intent around back to back reads and writes that I couldn't discern. The result was that back to back writes were getting dropped based on the value of current_io_operation if a new request came in before current_io_operation was set to IO_NONE in IDLE. I couldn't tell if this was intended to be some sort of write-to-write optimization but it was resulting in dropped writes as implemented. But that could have been an artifact of simulating with iverilog. I did also change it so that the rising edge of the read or write request triggers the operation because if the calling client held those too long, as is the case of my video generator that I'm using this with, then it would block subsequent requests as well.

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

2 participants