-
Notifications
You must be signed in to change notification settings - Fork 27
[rtl, gpio] Refactor gpio declaration #452
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
base: main
Are you sure you want to change the base?
Conversation
elliotb-lowrisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. I'd also be interested in seeing if the addition of more tlul_adapter_reg's in gpio.sv has affected the FPGA timing or area
| "gpio0", | ||
| "gpio1", | ||
| "gpio2", | ||
| "gpio3", | ||
| "gpio4", | ||
| "gpio5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to move these back up to where the single gpio was before? These are roughly in order of address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to make it to follow the same rules as uart, spi and i2c where every instance is listed individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, move these many instances up to where the single instance used to be in the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's not possible without hacking. That's because this file is autogenerated and the order is based on address offset.
| [[blocks]] | ||
| name = "gpio" | ||
| instances = 5 # RPi, Ard, Pmod0, Pmod1, PmodC | ||
| instances = 6 # RPi, Ard, Pmod0, Pmod1, PmodC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being changed? I thought only five of the six GPIOs went through the pinmux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this file should not only list IPs that relates to pinmux, but list all IP instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why does CI fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's because the manual changes that I have done in the RTL are wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @alees24 help, the test now pass. I should also test if the bitstream is correct
7b9135c to
82816d5
Compare
| input [31:0] gpio_ios_i [GPIO_NUM], | ||
| input [31:0] gpio_ios_en_i[GPIO_NUM], | ||
| output [31:0] gpio_ios_o [5], | ||
| input [31:0] gpio_ios_i [5], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change allows instances with direct ios (without going to pinmux). In this case we have 6 instances of gpio but only 5 goes to pinmux.
69156d0 to
c6220a8
Compare
c6220a8 to
4f34d60
Compare
Don't add empty pin list to the block_io_to_pin_map dictionary. Signed-off-by: Douglas Reis <doreis@lowrisc.org>
The gpio was as declared in some places as having a single instance, but in other places it had 6 instances, but in the xbar it only had one instance. This commit makes it consistent. Signed-off-by: Douglas Reis <doreis@lowrisc.org>
4f34d60 to
31c0161
Compare
The gpio was as declared in some places as having a single instance, but in other places it has 6 instances. This commit makes it consistent.
This change expand the number of instances of the GPIO block connected to the xbar.
This is the bitstream build report for these changes:
This is the same report from build on the main branch: