sifive: fix RX interrupt disable clobbering TX #3272
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The implementation of `chips::sifive::uart::Uart::disable_rx_interrupt` accidentally used `write` instead of `modify` when updating the interrupt enable UART register. This caused any time the RX interrupt was disabled in the code to also clobber and disable the TX interrupt, causing problems with transmit. Fix by using `modify` to clear RX interrupt bit.
bradjc
approved these changes
Oct 7, 2022
2 tasks
ppannuto
approved these changes
Oct 7, 2022
alistair23
approved these changes
Oct 9, 2022
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Oct 26, 2022
3283: sifive and hifive1: support 344MHz system clock r=bradjc a=gemarcano ### Pull Request Overview The SiFive FE310-G002 and FE310-G003 appear to have the same clock control hardware, and expose identical registers to manage the clocking of the system. This adds a ~~320MHz~~ (**edit: 344MHz**) clock frequency option, achieved by adjusting the PLL. Some additional code makes sure changing frequency is done safely. ~~UART needs to update its divider when the system clock changes to maintain a constant baud rate. A new function `Uart::update_clock_frequency` handles this, and should be called after a system clock change. Additionally, now `Uart::set_baud_rate` is called on creation to make sure the internal baud rate and hardware state is consistent.~~ **Edit:** UART now takes in the desired system clock on creation, so that the `set_baud` calculations take that into account. There is no need currently to do adjustments to the SPI peripheral serial clock divider. Default settings on the SPI peripheral divide the clock by 8, limiting the maximum frequency of the serial clock to 48MHz (maximum possible output from PLL peripheral is 384MHz). All QSPI chips listed as in use for HiFive boards support frequencies below 50MHz. I am also looking for feedback on how to make it more flexible to choose semi-arbitrary clocks (there are over 100 valid unique clock outputs from the PLL block), or if this is something that is unnecessary, and whether we should just support a small set of frequencies. A lot of the changes are comments just to make it easier to follow what is going on and why. I may have gone overboard, as I tend to err on the side of too much. ### Testing Strategy I tested this on a HiFive1 revB. It actually helped me catch a bug in the SiFIve UART driver, which should be fixed by #3272. I tested this by making sure UART communication happened, indicating that the baud rate was still properly clocked at 115200 even after adjusting the divider in the UART peripheral to account for the increased system clock. To confirm that the divider mattered, I removed the update logic to compensate for a change in the system clock, and I observed that UART communication was no longer possible (as expected). ### TODO or Help Wanted I checked the documentation for the G003 chip, and I believe the registers are all the same, and possibly even the PLL peripheral, but it would be really good to have someone with a FE310-G003 to check (I think that's a HiFive Inventor???). Additionally I have a few design questions, which I'll highlight in the comments. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Gabriel Marcano <gabemarcano@yahoo.com>
bors bot
added a commit
that referenced
this pull request
Jan 31, 2023
3283: sifive and hifive1: support 344MHz system clock r=lschuermann a=gemarcano ### Pull Request Overview The SiFive FE310-G002 and FE310-G003 appear to have the same clock control hardware, and expose identical registers to manage the clocking of the system. This adds a ~~320MHz~~ (**edit: 344MHz**) clock frequency option, achieved by adjusting the PLL. Some additional code makes sure changing frequency is done safely. ~~UART needs to update its divider when the system clock changes to maintain a constant baud rate. A new function `Uart::update_clock_frequency` handles this, and should be called after a system clock change. Additionally, now `Uart::set_baud_rate` is called on creation to make sure the internal baud rate and hardware state is consistent.~~ **Edit:** UART now takes in the desired system clock on creation, so that the `set_baud` calculations take that into account. There is no need currently to do adjustments to the SPI peripheral serial clock divider. Default settings on the SPI peripheral divide the clock by 8, limiting the maximum frequency of the serial clock to 48MHz (maximum possible output from PLL peripheral is 384MHz). All QSPI chips listed as in use for HiFive boards support frequencies below 50MHz. I am also looking for feedback on how to make it more flexible to choose semi-arbitrary clocks (there are over 100 valid unique clock outputs from the PLL block), or if this is something that is unnecessary, and whether we should just support a small set of frequencies. A lot of the changes are comments just to make it easier to follow what is going on and why. I may have gone overboard, as I tend to err on the side of too much. ### Testing Strategy I tested this on a HiFive1 revB. It actually helped me catch a bug in the SiFIve UART driver, which should be fixed by #3272. I tested this by making sure UART communication happened, indicating that the baud rate was still properly clocked at 115200 even after adjusting the divider in the UART peripheral to account for the increased system clock. To confirm that the divider mattered, I removed the update logic to compensate for a change in the system clock, and I observed that UART communication was no longer possible (as expected). ### TODO or Help Wanted I checked the documentation for the G003 chip, and I believe the registers are all the same, and possibly even the PLL peripheral, but it would be really good to have someone with a FE310-G003 to check (I think that's a HiFive Inventor???). Additionally I have a few design questions, which I'll highlight in the comments. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Gabriel Marcano <gabemarcano@yahoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Overview
The implementation of
chips::sifive::uart::Uart::disable_rx_interruptaccidentally usedwriteinstead ofmodifywhen updating the interrupt enable UART register. This caused any time the RX interrupt was disabled in the code to also clobber and disable the TX interrupt, causing problems with transmit. Fix by usingmodifyto clear RX interrupt bit.Specifically, this bug manifests way more frequently, and even apparently deterministically, when using system clock rates above the default of 16MHz. I was experimenting with the PLL, and configured the board to run at 320MHz (while taking care to also update the UART code to compensate for the increased system clock frequency), and pressing ENTER a few times was enough to trigger the bug. The bug would manifest as the expected output missing from
tockloader listen, unless one held down a key other than ENTER (effectively, firing the RX interrupt, which also let the TX handler portion run). I was not able to reproduce the issue at the default system clock of 16MHz.Testing Strategy
This was tested by flashing to a HiFive1 revB board and making sure that the pconsole worked fine by pressing ENTER repeatedly quickly (this was how I originally reproduced the bug, it would manifest as the
tock$from the pconsole disappearing).I additionally tested this PR by first increasing the system clock to 320MHz, adjusting the baud rate divider in the UART class to compensate for the faster system speed, and after building and flashing then pressing ENTER repeatedly to make sure the pconsole worked correctly.
TODO or Help Wanted
It would be nice to test this with the other SiFive board (HiFive Inventor?) to make sure it works fine.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.