-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fifo depth #224
Fifo depth #224
Conversation
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 looks reasonable to me. Not sure what the CI failures are about.
0ec6907
to
884fa67
Compare
Last force push was a rebase. |
Both the UART and I2C drivers only work with the large FIFO depth, fix incoming for this. |
Just converting this to draft while the fix for the drivers are in progress. |
dv/dpi/i2cdpi/i2c_hat_id.cc
Outdated
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xed, | ||
0x6e, 0x03, 0x00, 0x02, 0x00, 0x0e, 0x00, 0x00, // ........n....... | ||
0x00, 0x72, 0x70, 0x69, 0x2d, 0x73, 0x65, 0x6e, | ||
0x73, 0x65, 0x2d, 0x76, 0x32, 0x37, 0x54, 0xff // .rpi-sense-v27T. |
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.
The previous dump was missing half of the data due to a bug in the I2C driver (which is fixed with lowRISC/cheriot-rtos#8). This new dump is taken from the sense HAT I have and was obtained with the I2C on a raspberry-pi. Some bytes differ in the first half due to the different UUID and CRCs
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.
wow, good to have this fixed!
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.
The EEPROM test is failing in CI. Is this related to your rework?
Driver changes are in: lowRISC/cheriot-rtos@2c4c85b Unfortunately now hitting a failure in the CHERIoT RTOS test suite. Seems the write to stderr hangs which uses UART instance 1. Given stdout on UART 0 works fine and it's identical RTL and drivers this is a little weird. Waiting for a sim run with wave tracing to debug now. |
92d8589
to
a2b831b
Compare
Yet another failure! Seems my new RPI Hat EEPROM stuff only works with my sense HAT. There's a new temp commit to dump the EEPROM that you get from the HAT attached to the FPGA in CI. Also discovered some bugs in my original code that meant it was reporting a failure and the ignoring the failure and reporting a pass. I've fixed those and it all passes locally now, will wait for CI to divulge the EEPROM contents and hopefully produce something that will work with both Sense HATs. |
300f9e0
to
e76d2dc
Compare
Reduces I2C and UART FIFO depths to 8
Update code from upstream repository https://github.com/lowRISC/opentitan to revision 16bc8ecfaeca634081044eb43ea8975fd2901768 Signed-off-by: Greg Chadwick <[email protected]>
9919c1e
to
11cb1fe
Compare
Ok Sense HAT on the FPGA CI has a longer length in it's EEPROM header. I think it's one of the older versions that has a full compiled device tree overlay in the eeprom. With the length read from I2C extended (to 1024) this is now passing. So hopefully this will now pass CI and be good to go once the SW stuff is reviewed |
The EEPROM contains a UUID that is different for every board so a simple binary comparison will only ever work for a specific board. Furthermore there are a couple of sense HAT versions that cause further variations. With this rework we instead read and verify the EEPROM contents by using the CRCs that are part of the ID specification and check certain fields (like the vendor string) are as expected. This introduces some new utility functions for working with R-PI HAT EEPROMs that may be useful for other Sonata software.
In particular this gives us the updated I2C driver that reflects the FIFO changes.
One final change, the EEPROM test data in the I2C DPI is now what I captured from the FPGA in CI |
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.
Had a quick look through and it looks good! A good quality piece of yak shaving.
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.
Thanks for getting this sorted. Much more difficult than initially thought. Great to have this in!
This only touched the vendored in IP, the SPI FIFO sizes will be addressed via #198.
The I2C is also using a memory back FIFO so I don't think changes there have much impact on FPGA utilization, however I think it's important we have some uniformity in terms of having small FIFOs everywhere.
All builds fine in Vivado, running some tests on the actual board now with the resultant image.