Skip to content

Conversation

mciantyre
Copy link
Contributor

@mciantyre mciantyre commented Dec 28, 2020

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

  • fix a GPT clock gating bug introduced in Add Teensy 4 board #2200
  • move UART root clock initialization out of the configure() method, and into CCM peripheral setup

Testing 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 needs min_const_generics accepting as-is, and we'll remove the feature later
  • figure out a way to remove the remaining static peripheral in iomuxc_snvs
  • remove std dependency in new unit tests, avoiding the conditional no_std in the crate
  • leave thoughts on GPIO changes
  • clean up commit messages

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

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.
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.
hudson-ayers
hudson-ayers previously approved these changes Dec 29, 2020
Copy link
Contributor

@hudson-ayers hudson-ayers left a 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.

@mciantyre mciantyre marked this pull request as ready for review December 29, 2020 22:09
hudson-ayers
hudson-ayers previously approved these changes Dec 30, 2020
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.
@bradjc bradjc added the last-call Final review period for a pull request. label Jan 4, 2021
@bradjc
Copy link
Contributor

bradjc commented Jan 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 5, 2021

@bors bors bot merged commit c6d9f81 into tock:master Jan 5, 2021
@mciantyre mciantyre deleted the imxrt-periph-inst branch January 5, 2021 20:26
bors bot added a commit that referenced this pull request Feb 9, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call Final review period for a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants