-
-
Notifications
You must be signed in to change notification settings - Fork 766
Board based instantiation of chip drivers and interrupt mappings: imxrt10xx #2311
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
Conversation
A GPIO port already knows what kind of port it is. Passing in the GPIO port value to handle_interrupt is redundant. The commit removes the Port::handle_interrupt argument, and discerns the GPIO port using a check for pointer equality.
Previous work added the GPT2 static instance. However, the new instance was still referring to the GPT1 clock gate. This commit corrects that bug. We expose the GPT2 clock methods from the CCM, add a new clock gate enumeration, then correctly instantiate the GPT instances with the independent clock gates. The commit also correct for an unused lifetime in the gpt typdefs.
Working through the new peripheral initialization API, I realized that the GPIO driver wasn't amenable for the new API. The driver was using static, mutable memory internally, which would make it difficult to adapt to the new board-based peripheral initialization. This commit tries to refactor the GPIO driver. I took the opportunity to simplify some of the code, and address some potential bugs in the implementation. I'll call out the possible issues in code review. I tried to follow the "don't model the STM32 GPIO driver" guidance I found in the code. This refactored driver should be a good compromise that lets us move towards the board-based peripheral init API. Teensy 4 light continues to blink from this commit. Kernel size before this commit: make -C boards/teensy40 text data bss dec hex 62257 9356 12148 83761 14731 Size after: text data bss dec hex 60853 7884 13620 82357 141b5
The commit lays the foundation for the board-based peripheral initialization API. TODOs from this commit: - remove static CCM instance - remove static IOMUXC_SNVS instance - fix UART clock control
We need to disable _all_ UART clocks at the clock gates before we change the UART clock root. Before this commit, the configure method would disable the clock gate for the current instance, configure the clocks for _all_ UART instances, then re-enable its own clock. Though the setting was the same for each configure call, so there were probably no ill effects, we shouldn't configure the UART clock root in each instance's configure call. This commit moves the UART root clock configuration into each board's early setup routine. This should be equivalent to what we were doing before. The 'disable' term for the clock mux is misleading. This call actually transitions the UART to use PLL3, not disable the clock to the UART. (If the PLL is off, that's not obvious from this call.) The commit refactors the CCM API to expose setters and getters.
Thread references to the CCM into other peripherals so that the CCM clock gates don't need to refer to a static CCM instance.
Reduced size by a few hundred bytes.
They're now unused, so we can reduce LOCs.
db8193f
to
d664c2c
Compare
This removes the Pins trait from the previous commits. We can simplify the implementation, and keep both PidId GPIO offset APIs, without that extra implementation.
Error messages won't be too helpful, but this component shouldn't change much at this point.
Remove SNVS pad responsibility from IOMUXC module. Instead, directly use the IomuxcSnvs type.
d664c2c
to
dcb02f7
Compare
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 good to me, thanks for taking this on!
I think that #2310 may take awhile, as we need to rewrite a decent chunk of assembly code to get rid of warnings. But I think that we are ok accepting PRs with min_const_generics
now that it is stabilized, and we can just remove the feature flag once #2310 is merged -- so feel free to mark this as ready for review if you do not have anything else you plan to change.
The Imxrt10xxDefaultPeripherals requires the board user to already allocate the CCM instance. It's passed in on construction. We don't need to make a duplicate instance in the default peripherals.
bors r+ |
2400: Double stack buffer for i.MX RT-based boards, prune panic! allocations r=bradjc a=mciantyre ### Pull Request Overview This pull request doubles the stack buffer for boards that use i.MX RT chips. #2311 added board-based peripheral initialization for i.MX RT boards. We use more stack space (at least ~2KiB, most memory for GPIO pins) to initialize the static peripherals, which might exceed the stack buffer once the kernel starts running. Doubling the stack size seems to suffice for running the kernel and two applications on a Teensy 4.0. This approach is similar to #2223, which noted the issue in STM32 boards. I first bisected the issue down to a commit in #2334. After that compiler update, I couldn't run the Tock kernel on the Teensy 4.0. But it turns out that, after #2311 and before the updated compiler, a `panic!()` would overflow the stack. So this seems to be a latent issue that manifested with a compiler update. The boards' panic handlers were allocating large GPIO ports structures just to access a single LED. This PR fixes that by exposing a constructor for individual GPIO pins, and using that constructor in the panic handler. ### Testing Strategy This pull request was tested by rebasing onto trunk, compiling the kernel for a Teensy 4.0, and using the kernel to run two `libtock-c` example apps. Then, I added a `panic!()` just before the main kernel loop, and showed that the panic handler worked as expected. ### TODO or Help Wanted There may be other boards that allocate large objects (GPIO ports and pins) in the panic handler. See the [stm32f3discovery board](https://github.com/tock/tock/blob/86079650ae9d2e5b7287571a2219050c76aa3f1c/boards/stm32f3discovery/src/io.rs#L72) and the [stm32f412gdiscovery board](https://github.com/tock/tock/blob/86079650ae9d2e5b7287571a2219050c76aa3f1c/boards/stm32f412gdiscovery/src/io.rs#L74) for two examples. Are we aware of this behaviors in these boards, or in other boards? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Ian McIntyre <ianpmcintyre@gmail.com>
Pull Request Overview
The PR refactors the
imxrt10xx
chip and boards to support board-based initialization (first proposed in #2069). It builds on the work of #1918 and #2200, where we noted the effort as a TODO. After this PR, you may instantiate and configure i.MX RT peripherals in a board, or use the default peripherals provided by the chip.Most peripherals were easily transitioned to the new API. The exception was the GPIO driver, which referenced
static
GPIO ports and pins throughout the code. I may have found some issues in the driver, so I took the refactoring opportunity to update the implementation, trying to follow this suggestion. I'll leave more details in review comments.Other changes include
configure()
method, and into CCM peripheral setupTesting Strategy
Tested
boards/teensy40
on a Teensy 4.0 using the tests from #2200.boards/imxrt1050-evkb
continues to compile, but I don't have the hardware to test examples.TODO or Help Wanted
Looking for feedback on the refactor. Let me know if it deviates too much from the other chips.
Holding as draft to
wait for update rust to nightly-2020-12-28 #2310, since the new GPIO driver needsaccepting as-is, and we'll remove the feature latermin_const_generics
static
peripheral iniomuxc_snvs
std
dependency in new unit tests, avoiding the conditionalno_std
in the crateDocumentation Updated
Updated the relevant files inno updates are required./docs
, orFormatting
make prepush
.