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

Configurable UART #57

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AgamemnonasKyriazis
Copy link

I developed this configurable UART module as part of my thesis, I had opened a previous pull request but when I tried to re-base I had some issues I couldn't solve. Also I finished the document of the thesis, which I will include here.

document

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for submitting this PR. We really appreciate you using the Ibex demo system for your thesis and sharing your results with us. I'm not sure if this is possible to merge in in its current state. For example, we usually use .sv files instead of .v files. I've added some initial comments that I went though, but I can totally understand if you have limited bandwidth to do more work on this at this time. Again, thank you for your time and engagement.

ibex_demo_system.core Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you copying this from sw/common/?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy bug, I had a version without the c/rust update and it resulted to this when I copied the folder.

…e,[core] Restored original ibex_demo_system.core the change was a mistake because of the board I used to load the bitstream
@AgamemnonasKyriazis
Copy link
Author

I removed link.ld file and restored the original ibex_demo_system.core file. I understand that the whole project is written in SystemVerilog. Given I have the time and want to be part of this project, I consider rewriting the whole module in SystemVerilog. In general are you interested in the concept of a configurable UART module? Also is there anything else you would like to see included?

@marnovandermaas
Copy link
Contributor

I removed link.ld file and restored the original ibex_demo_system.core file. I understand that the whole project is written in SystemVerilog. Given I have the time and want to be part of this project, I consider rewriting the whole module in SystemVerilog. In general are you interested in the concept of a configurable UART module? Also is there anything else you would like to see included?

@GregAC can you have a look at this?

+ Almost full flag for rx/tx sync FIFO
+ Almost empty flag for rx/tx sync FIFO
+ Almost full interrupt for tx in C
+ Almost full / empty flag added to read instruction
missed to declare the enable_tx_int routine, also pushed a version where the interrupt is commented out...
[sw] added the volatile qualifier to avoid optimization of empty for block
@marnovandermaas
Copy link
Contributor

I'm sorry for the long delay on responding to this. Although in principle we like the idea of a configurable UART module, I think this PR may require quite a lot of rework and reviewing from our part. This is currently not one of our top priorities. Please do not put too much more effort into this PR as we are unlikely to have a look at this any time soon. Thank you for your interest in the Ibex demo system and we think it is great that you have used and extended it.

@marnovandermaas marnovandermaas marked this pull request as draft October 10, 2023 16:04
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