Board-based instantiation of chip drivers and interrupt --> driver mapping for Apollo3 + SAM4L

[hudson-ayers]

Pull Request Overview

This pull request proposes a new design for managing chip drivers and interrupts.
The new design makes boards responsible for instantiating chip drivers, and for defining the mapping from interrupt numbers --> drivers.
It also adds the InterruptService trait to kernel/src/platform/mod.rs, as it will no longer only be used by nordic boards.

I would appreciate feedback before I implement this for all chips/boards, and answers to my outstanding questions below.

Advantages of the design:

• makes it possible for boards to exclude code for chip drivers that they are not using (e.g. solves Unused chip drivers still take up RAM #625)
• makes boards responsible for static initialization of all chip drivers, much in the same way that boards are responsible for initializing capsules, which removes reliance on global static mut variables in these scenarios (a step in the right direction for static mut data considered harmful (as opposed to static_init) #1545 / Missed optimization opportunities with static mut data? #1662 ).
• If so desired, out-of-tree board authors can write chip drivers that live in the boards crate, and still use the upstream chip crate.
• introduces macros that live in chips/ to define the default interrupt mapping, so that upstream boards will not have to reimplement interrupt -> driver mappings in the common case.
• removes all dependence on const_fn throughout Tock, except for in tock-register-interface. Most uses of const_fn in Tock today are only required because of the widespread practice of initializing drivers as globals. By transitioning away from this design, most constructors can be made non-const, a big step towards Building Tock with stable rustc #1654 .
• I think these changes would enable OpenTitan to use the upstream earlgray crate, and maintain only a board crate out-of-tree, which is probably a win for both projects.

So far, this PR is only implemented for the apollo3/redboard_artemis_nano combination (I picked it because there aren't very many apollo3 drivers at this point, so the total work was smallest).

I have verified that this PR excludes chip driver code in a way that is not currently possible on master, by removing the ble and i2c drivers from the board. Unfortunately removing const_fn everywhere seems to have introduced some code size cost.

Testing Strategy

This pull request was tested by compiling.

TODO or Help Wanted

Outstanding Questions

1. How to handle deferred calls? I propose that they should be handled identically to interrupts in this design, that is handle_deferred_call() should be added to the InterruptService trait, and populated by a macro for the common case. -- Done.

2. Is modifying the Platform trait ideal, or would it be better to use basically the InterruptService trait suggested in [RFC] trait for servicing interrupts #1501, which could be implemented for the Platform, or for a struct containing just the drivers? I decided that using InterruptService is clearer and makes more sense than conflating Platform with interrupt mapping. This technique allows for chips to still define some set of required interrupt mappings if it wishes, and should make adapting this to the nordic boards easier.

3. Is it in fact desirable to remove all uses of #[const_fn], or should I remove that portion of this PR and save it as a followup PR? It seems to me that const constructors would be preferable if the feature was stable, so maybe I am jumping the gun on this. Decided to revert the const removals, for now.

4. Some drivers do not handle interrupts (mcuctrl, pwrctrl, clkctrl in the Apollo3 case), but still do not need to be initialized as globals. I propose that when these drivers are only used in main.rs, they should simply be stack allocated in reset_handler(). If used in chip.rs, they should be made fields on the respective chip struct, and initialized with the Chip. This makes it clear which drivers are necessary for chip operation vs. optionally included by boards. In fact, this approach could be taken one step further, by making "required" drivers fields of the chip struct. Things like uart and timer which are prerequisites for including a board in Tock could be placed on the chip struct.

Documentation Updated

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

Formatting

• Ran make prepush.

[bradjc]

I was skeptical at first, but now I think this is a very nice change that I would like to see merged.

My main comment is just around the naming. I think using "peripheral" instead of "driver" would be less confusing, since Driver is the trait for implementing a syscall. I also think the macro name should include "default".

1. Agreed.
2. Makes sense.
3. no comment
4. This would all still happen in the macro wouldn't it? So our current boards wouldn't see a difference? I'm not sure we need to specify anything at this point.

[bradjc]

I think it would also make sense to start a "layer responsibilities" doc that describes what various layers are tasked with doing, and why. I think this PR adds a rather nonintuitive responsibility for board crates.

[silvestrst]

@hudson-ayers thank you for this PR!

I have actually played around with the idea myself when working on the OpenTitan chip_config.rs, as it is cleaner and more flexible. I have eventually dropped the idea, because it seemed to costly for the benefit of chip_config alone. You have found stronger reasons, more useful use-cases, making this change more justifiable. I think this is a great idea, thanks again!

Tiny thing, which I think is not an issue is that if board decides to override a driver - the default chip driver will still be built. I guess that is not much of an issue, as this does not affect the final image size, just increases the intermediate library size and the compilation time. However, I imagine overriding default drivers will probably be a rare occasion, which makes this a non-issue?

[silvestrst]

I think that register definitions should probably be split out in a separate .rs, as they remain the same regardless what driver we use. Allows to avoid reinventing the wheel for the "board" drivers.

[bradjc]

I think that can be done on a per-chip basis as needed. Having everything in one place is convenient in the common case.

[silvestrst]

For earlgrey, I think it is worth now initialise the chip_config::Config, from chip_config.rs in the corresponding board and passing the structure as a parameter into new.

For OpenTitan it will also allow to remove the following features from the .../earlgrey/Cargo.toml:

[features]
config_fpga_nexysvideo = ["config_disable_default"]
config_sim_verilator = ["config_disable_default"]
config_disable_default = []

And instead have the config related features entirely in the board Cargo.toml.

P.S.

I know this is apollo related change, but seems like the most appropriate place to leave this comment.

[silvestrst]

I think that base address, together with the register definitions should probably be split out in a separate .rs, as they remain the same regardless what driver we use. Allows to avoid reinventing the wheel for the "board" drivers.

Basically decouple implementation from the register and base definitions.

[silvestrst]

I think apollo3_driver_definitions would need to be passed into the chip, as the chip might want internally use a driver which has been overridden by the board. Otherwise we would have a mismatch of board using one driver implementation, and the board another.

In this case the macro below, probably could just be a function, something like apollo3_default_drivers_get or something similar.

[hudson-ayers]

I think that if a chip needs to use a driver internally, that driver should be a field on the Chip struct itself, revealing that the Chip depends on that specific driver (and thereby showing that driver is not optional). If we want that driver to further be replaceable by multiple implementations, then introducing a trait for that particular driver would be appropriate. But I think we want to make it explicitly not possible for the chip to depend on the particular contents of whatever struct implements InterruptService, and instead depend only on the interface exposed by InterruptService.

[bradjc]

It seems like having that driver be an argument to chip::new() is reasonable to handle this case.

[hudson-ayers]

I have added support for deferred calls, and implemented this for the sam4l + Imix. Doing so ended up requiring..a lot of code changes. I will do this for hail next, but after that I think it might be best to get this in as separate PRs for each subsequent chip.

Imix code size report from before this PR:

   text    data     bss     dec     hex filename
 174081    3272   21304  198657   30801 /home/hudson/tock/target/thumbv7em-none-eabi/release/imix

Imix code size report from after this PR:

   text    data     bss     dec     hex filename
 176641      64   32704  209409   33201 /home/hudson/tock/target/thumbv7em-none-eabi/release/imix

Noticeably, almost nothing left in .data, which is pretty cool to see, though much of that size appears to be moved into .text. But now it is possible to build a minimal board without the many sam4l drivers!

For a minimal "Imix_mini" board that only supports gpio + led + timer, with no modifications to the sam4l crate:

with this PR:

   text    data     bss     dec     hex filename
  52737       0   16384   69121   10e01 /home/hudson/tock/target/thumbv7em-none-eabi/release/imix_mini

Function groups (flash): 31491 bytes
  capsules::*          2418 bytes
  compiler_builtins::* 76 bytes
  components::*        304 bytes
  core::*              11655 bytes
  cortexm4::*          2200 bytes
  cortexm::*           1720 bytes
  imix_mini::*         288 bytes
  kernel::*            11628 bytes
  sam4l::*             1202 bytes

before this PR:

   text    data     bss     dec     hex filename
  61953    3148   13232   78333   131fd /home/hudson/tock/target/thumbv7em-none-eabi/release/imix_mini2

Function groups (flash): 40379 bytes
  capsules::*          2414 bytes
  compiler_builtins::* 76 bytes
  components::*        308 bytes
  core::*              11947 bytes
  cortexm4::*          2200 bytes
  cortexm::*           1720 bytes
  imix_mini2::*        368 bytes
  kernel::*            16152 bytes
  sam4l::*             5100 bytes
  tock_cells::*        74 bytes
  tock_registers:      20 bytes

This was a rough/quick comparison, and does not exclude debug machinery or the power manager in either example. But it is at least clear that excluding drivers can provide meaningful benefit for code size, both by removing code size from drivers, and by reducing the portion of the core kernel code that is actually included in the final binary.

[brghena]

Is the data just going into .bss instead? In the imix_mini example, it grows by the exact amount data shrinks, plus 4.

[hudson-ayers]

Is the data just going into .bss instead? In the imix_mini example, it grows by the exact amount data shrinks, plus 4.

I am not sure..is this maybe an MPU alignment thing, where the RAM section is always going to be a power of 2, and the leftover RAM is given to uninitialized variables?

Have some numbers:

imix_mini2 (current master):

(.venv) hudson: ~/tock (no-more-globals) $ ./tools/print_tock_memory_usage.py --depth=3 -v target/thumbv7em-none-eabi/release/imix_mini2.elf
Tock memory usage report for target/thumbv7em-none-eabi/release/imix_mini2.elf
Kernel occupies 64852 bytes of flash
   61704        code and constant strings
    3148        variable initializers
Kernel occupies 16380 bytes of RAM
    8192        stack
    5040        uninitialized variables
    3148        initialized variables
    8188        variables total
Applications allocated 0 bytes of RAM

Variable groups (RAM): 3942 bytes
  imix_mini2::CHIP:                     4 bytes
  imix_mini2::PROCESSES:                32 bytes
  imix_mini2::io::WRITER:               1 bytes
  imix_mini2::reset_handler::BUF1:      24 bytes
  imix_mini2::reset_handler::BUF2:      20 bytes
  imix_mini2::reset_handler::BUF::*     300 bytes
  kernel::common::deferred_call:        4 bytes
  sam4l::adc::ADC0:                     52 bytes
  sam4l::aes::AES:                      40 bytes
  sam4l::ast::AST:                      12 bytes
  sam4l::crccu::CRCCU:                  548 bytes
  sam4l::dma::DMA_CHANNELS:             384 bytes
  sam4l::dma::NUM_ENABLED:              4 bytes
  sam4l::flashcalw::FLASH_CONTROLLER:   32 bytes
  sam4l::gpio::INTERRUPT_COUNT:         4 bytes
  sam4l::gpio::PA:                      516 bytes
  sam4l::gpio::PB:                      516 bytes
  sam4l::gpio::PC:                      516 bytes
  sam4l::i2c::I2C0:                     72 bytes
  sam4l::i2c::I2C1:                     72 bytes
  sam4l::i2c::I2C2:                     72 bytes
  sam4l::i2c::I2C3:                     72 bytes
  sam4l::pm::PM:                        8 bytes
  sam4l::spi::SPI:                      32 bytes
  sam4l::trng::TRNG:                    12 bytes
  sam4l::usart::IS_PANICING:            1 bytes
  sam4l::usart::USART0:                 56 bytes
  sam4l::usart::USART1:                 56 bytes
  sam4l::usart::USART2:                 56 bytes
  sam4l::usart::USART3:                 56 bytes
  sam4l::usbc::USBC:                    368 bytes
   ! 7 bytes of data or padding after sam4l::usart::IS_PANICING
   ! 3 bytes of data or padding after imix_mini2::io::WRITER
Total of 10 bytes wasted in RAM

Embedded data (flash): 15229 bytes

imix_mini (this PR):

(.venv) hudson: ~/tock (no-more-globals) $ ./tools/print_tock_memory_usage.py --depth=3 -v target/thumbv7em-none-eabi/release/imix_mini.elf
Tock memory usage report for target/thumbv7em-none-eabi/release/imix_mini.elf
Kernel occupies 52560 bytes of flash
   52560        code and constant strings
       0        variable initializers
Kernel occupies 16384 bytes of RAM
    8192        stack
    8192        uninitialized variables
       0        initialized variables
    8192        variables total
Applications allocated 0 bytes of RAM

Variable groups (RAM): 1966 bytes
  imix_mini::CHIP:                  4 bytes
  imix_mini::PROCESSES:             32 bytes
  imix_mini::io::WRITER:            1 bytes
  imix_mini::reset_handler::BUF1:   24 bytes
  imix_mini::reset_handler::BUF2:   20 bytes
  imix_mini::reset_handler::BUF::*  1876 bytes
  kernel::common::deferred_call:    4 bytes
  sam4l::gpio::INTERRUPT_COUNT:     4 bytes
  sam4l::usart::IS_PANICING:        1 bytes
   ! 4 bytes of data or padding after imix_mini::reset_handler::BUF
   ! 2 bytes of data or padding after sam4l::usart::IS_PANICING
Total of 6 bytes wasted in RAM

Embedded data (flash): 13164 bytes

Clearly some of .data has moved to .bss, such as the gpio::PA struct. But I don't see why, say, there would be space for an unitinitialized USBC struct, unless I have done something wrong.

[bradjc]

Hmm...given that the sam4l has 8x more flash than RAM, this seems like a change we don't want.

RAM usage goes up by (32704-21304) - (3272-64) = 8192 bytes.

[hudson-ayers]

Hmm...given that the sam4l has 8x more flash than RAM, this seems like a change we don't want.

RAM usage goes up by (32704-21304) - (3272-64) = 8192 bytes.

I think this is just an artifact of MPU alignment. If i decrease the stack size on Imix by 100 bytes, then the bss size drops to 24512 (a decrease of 8192 bytes, leaving the total RAM usage the same as without this PR). So the impact of this change on RAM usage is much smaller than that in reality.

Note: The MPU minimum alignment for the sam4l is 8K.

[hudson-ayers]

I think this is ready for review, for apollo3 + sam4l. I have reverted the const_fn removal based on the core call discussion today.

By playing with the stack size on current master and this PR to see what stack size marks the boundary where RAM use increases by 8k, I have determined that this PR actually increases memory usage by about 175 bytes.

One thing to pay attention to: constructors of chip peripherals are now public, so they can be called from boards. This has the potential to be a bit of a footgun, as only one instance of the peripheral can receive interrupts. We could potentially prevent this with a capability, but for now I think we should just see how it works out.

[alistair23]

I'm not a fan of the giant macro but otherwise I think this is a good improvement. The current way of handling interrupts has always annoyed me and I think this is a bit better.

I reviewed the Apollo3 parts, no the SAM4L though.

[hudson-ayers]

I have removed the large macros, and just use structs defined in chips.rs instead. I also added a little documentation to Porting.md.

[phil-levis]

Can you fix the merge conflicts?

[alistair23]

Looks good for the Apollo3

[hudson-ayers]

Can you fix the merge conflicts?

done

[phil-levis]

Some new conflicts, resolve and I'm ready to approve.

[bradjc]

@hudson-ayers Have you verified that the non-macro approach does make a smaller binary if you do not use all peripherals?

[hudson-ayers]

@hudson-ayers Have you verified that the non-macro approach does make a smaller binary if you do not use all peripherals?

Just did, the results seem pretty much the same as using the macro approach -- still significant savings.

Merge conflicts have been addressed.

[bradjc]

bors r+

[bors]

Build failed:

• ci-qemu

[hudson-ayers]

looks like a network connection error.

bors retry

[bors]

Build failed:

• ci-qemu

[hudson-ayers]

https://git.qemu.org/ is down, so the QEMU CI cannot pass. But neither of these chips is one of the QEMU tested chips anyway, so I think this is safe to manually merge.

[alistair23]

That's a pain. We could use the GitHub mirror instead?

[hudson-ayers]

Given that we already have to rely on Github being up to run CI, that would probably be an improvement. This may be a sufficiently rare occurrence it isn't worth investing the time to switch it though.