Out of tree (driver) configuration support

[thomasstenersen]

Summary

This PR mainly defines and adds support for out-of-source drivers. Specifically in the context of using the BLE controller in https://github.com/NordicPlayground/nrfxlib and https://github.com/NordicPlayground/fw-nrfconnect-nrf, but the approach should be generic for other similar use cases where an out-of-source subsystem needs specific control over certain drivers/peripherals.

In addition the PR has some minor fixes for making Zephyr play nicer with "outside" configuration.

8f1e6cb should provide some explanation and reasoning for the main changes in this PR.

FAQ

Answering the question you might have when seeing this PR.

Do we really need this? Isn't there another way?

Yes we need this, unfortunately. This is for the case were an out-of-tree component for some reason must use it's own driver. There is no way in Kconfig that I and others have found to effectively de-select a Kconfig choice/config without severely polluting the Zephyr configuration files.

Are we going to have such a kconfig for each driver subsystem?

I would add it at a per use-case/need basis. But this is a generic pattern, yes.

Are we talking about 2 conflicting drivers and we would select the one out of tree instead of the one in the tree?

Yes. And the fact that they are not only conflicting but another subsystem must use the out-of-tree one. We also want this switch to happen automatically, so if I select a different BLE controller in the menuconfig, the correct driver dependencies are resolved without the need for other changes by the user.

In short, this looks like a hack to me.

I would like it for there to be a nicer way of doing this. But as far as I can tell, this is the most pragmatic, least intrusive and nicest way of handling out-of-tree dependencies like this. IMO the only way of actually solving the root problem is to resolve #8181.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[codecov-io]

Codecov Report

Merging #13631 into master will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13631      +/-   ##
==========================================
+ Coverage   51.97%   52.12%   +0.14%     
==========================================
  Files         308      308              
  Lines       45512    45527      +15     
  Branches    10546    10552       +6     
==========================================
+ Hits        23656    23732      +76     
+ Misses      17055    16990      -65     
- Partials     4801     4805       +4

Impacted Files	Coverage Δ
boards/posix/nrf52_bsim/bstests_entry.c	57.6% <0%> (-3.27%)	⬇️
drivers/clock_control/nrf_power_clock.c	50% <0%> (-2.11%)	⬇️
ext/hal/nordic/nrfx/hal/nrf_rng.h	100% <0%> (ø)	⬆️
subsys/bluetooth/controller/hci/hci.c	52.66% <0%> (+0.08%)	⬆️
subsys/bluetooth/host/hci_core.c	44.19% <0%> (+0.12%)	⬆️
kernel/sched.c	84.32% <0%> (+0.31%)	⬆️
drivers/timer/nrf_rtc_timer.c	92.53% <0%> (+0.6%)	⬆️
arch/posix/core/posix_core.c	91.91% <0%> (+1.01%)	⬆️
boards/posix/nrf52_bsim/trace_hook.c	48.78% <0%> (+4.87%)	⬆️
boards/posix/nrf52_bsim/time_machine.c	55.73% <0%> (+8.19%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c78c5e6...8e8c9fe. Read the comment docs.

[SebastianBoe]

I think we want a different name for this option.

It's especially the 'SUBSYS' part that I'm concnerned about. I understand that in this particular case, an out-of-tree subsystem needed to force a particular driver.

But it might be in the future that an out-of-tree driver, or out-of-tree app might want to force their driver. So the 'subsys' term doesn't work. Perhaps

CLOCK_CONTROL_HAS_OOT_DRIVER
CLOCK_CONTROL_HAS_FORCED_DRIVER
CLOCK_CONTROL_HAS_OUTSIDE_DRIVER

I'd rather not use the term 'external' as it is not as precise as out-of-tree.

[thomasstenersen]

I agree, the name is not the best. I don't mind long names here, so something CLOCK_CONTROL_HAS_OUT_OF_TREE_DRIVER is fine by me. Other ideas/suggestions are welcome as well.

[b0661]

I assume "CLOCK_CONTROL_HAS_SUBSYS_DRIVER" and the "if CLOCK_CONTROL_HAS_SUBSYS_DRIVER" guard was introduced to allow the OOT driver and the Zephyr driver to use the same Kconfig symbols without interfering each other.

In my view the usage of the same Kconfig symbols should be controlled by the driver. The Zephyr driver or the OOT driver should enable the Kconfig symbols by a non user selectable Kconfig option (e.g. CLOCK_CONTROL_NRF_HAS_DRIVER).

A driver that wants to activate the options just has to select CLOCK_CONTROL_xxx_HAS_DRVER.

menuconfig CLOCK_CONTROL_MY_NRF
	bool "My NRF Clock controller support"
	depends on SOC_COMPATIBLE_NRF
        select CLOCK_CONTROL_NRF_HAS_DRIVER
	help
	  Enable my specific support for the Nordic Semiconductor nRFxx series SoC clock driver.

menuconfig CLOCK_CONTROL_NRF
	bool "NRF Clock controller support"
	depends on SOC_COMPATIBLE_NRF
        select CLOCK_CONTROL_NRF_HAS_DRIVER
	help
	  Enable support for the Nordic Semiconductor nRFxx series SoC clock
	  driver.

if CLOCK_CONTROL_NRF_HAS_DRIVER

config CLOCK_CONTROL_NRF_IRQ_PRIORITY
	int "Power Clock Interrupt Priority"
	range 0 7
	default 1
	help
	  The interrupt priority for Power Clock interrupt.

  ...

By this there is no need for the if guard and it would also allow to keep the Kconfig option for the driver mostly in one place.

[thomasstenersen]

I assume "CLOCK_CONTROL_HAS_SUBSYS_DRIVER" and the "if CLOCK_CONTROL_HAS_SUBSYS_DRIVER" guard was introduced to allow the OOT driver and the Zephyr driver to use the same Kconfig symbols without interfering each other.

It should be if !CLOCK_CONTROL_HAS_SUBSYS_DRIVER.
This is not the reason. The driving use-case for this is for out-of-tree subsystems (or anything, really) to have a way of de-selecting in-tree drivers. This could be an external subsystem that is Zephyr agnostic and needs all flash, RNG, etc. operations to be done through its own APIs. Keeping the drivers tied to a particular subsystem could, for example, be to meet real-time and/or security requirements or because of hardware constraints.

Furthermore, we don't want to pollute the Zephyr config files, drivers, etc. with out-of-tree specific configuration and code.

Let's say that I have a subsystem FOO that when selected needs to use it's own driver FLASH_DRIVER_FOO. The problem arises when the user selects CONFIG_FOO=y, FOO has no way of saying that, e.g., FLASH_DRIVER_NRF is incompatible with it.

I hope this cleared things a bit up, let me know if there is something I missed or didn't explain well enough.

[SebastianBoe]

Let's go for CLOCK_CONTROL_HAS_OUT_OF_TREE_DRIVER

[thomasstenersen]

I agree on the semantics of HAS vs MUST, but I'd argue that naming consistency is more valuable. <DRIVER>_HAS_OUT_OF_TREE_DRIVER it is,

[SebastianBoe]

But what about when in the future we HAVE an out-of-tree driver, but don't want to force it to be enabled?

What do you mean about naming consistency? Usually, HAS is used to indicate that something supports something, not that one must use something.

AFAICT HAS and MUST_USE indicate two different situations and need two different naming schemes.

[thomasstenersen]

If you have a driver but it's not enforced, you'd add it to a choice selection. Then you can select driver at will and you shouldn't use this mechanism. I don't think that is an argument here.

By consistent naming I'm talking about using HAS to resolve depends on dependencies in a sane way. I would like to keep different names to a minimum, i.e., remove usage of SUPPORTS and the like. The I don't have to ask myself every time: "was it <FOO>_HAS_<BAR> or <FOO>_SUPPORTS_<BAR>, ..."

That being said, I do acknowledge that this case is different though, is it different enough to warrant its own convention?

[SebastianBoe]

Yes.

[thomasstenersen]

Changed

[aescolar]

@SebastianBoe
Overall: this PR is ok for us in the sense that it does not harm our needs, and may help if we need to avoid some in-tree drivers kconfig files being included.
But it does not yet address the main issue.
That main issue being how do we get out of tree folders to be part of what cmake and Kconfig go thru (similar to the BOARD/SOR/ARCH_ROOT options), but for any arbitrary set of component.
There is some thoughts here about the possibility of having a way to define an "overlay" folder structure, that could contain all the needed extra components following the same folder structure as in the zephyr root, but only containing the folders that need replacing/extending (so what cmake and kconfig would traverse would be something like an union of both the zephyr root and that overlay root)
As said, hopefully @kapedev will soon engage with this. It is very nice to see you guys have similar needs, hopefully we will be having a nice solution that covers both of our needs.

[SebastianBoe]

That main issue being how do we get out of tree folders to be part of what cmake and Kconfig go thru (similar to the BOARD/SOR/ARCH_ROOT options)

Note that this recently became possible through west. Out-of-tree CMake and Kconfig code can be sourced as if it were an in-tree subsys. An example of this in action can be seen below.

This patch to west.yaml:

https://github.com/zephyrproject-rtos/zephyr/pull/13672/files#diff-4ae7a6d2c692e706a33f7a71d3eb25d2R38

causes the files (zephyr/CMakeLists.txt and zephyr/Kconfig) in another repo to be add_subdirectory'd / sourced:

https://github.com/JuulLabs-OSS/mcuboot/pull/430/files#diff-83aeef700a5a192d68e562d9ccfe3725R1

But this is not quite as powerful as the overlay you describe, as you can't inject out-of-tree CMake/Kconfig code into a specific in-tree CMake directory scope, or an in-tree Kconfig source-hierarchy.

An overlay mechanism would be interesting, but I'm not sure how one could implement it in a sane way.

[thomasstenersen]

Fixed merge conflicts and updated nrf52_bsim Kconfig.board as @aescolar pointed out. Is there anything left for me to do here or can we merge this @aescolar @Vudentz?

[aescolar]

@thomasstenersen : I'm ok with it. But I do not own/maintain most of it. So I let the approvals for those who do.

[carlescufi]

I am fine with this except for the notation:

*_MUST_USE_OUT_OF_TREE_DRIVER is simply too long and verbose.
I propose either:

• *_API_ONLY
• *_EXT_DRIVER
• *_OOT_DRIVER

Thoughts?

[ulfalizer]

Just some random style nits (on vacation :). Could take a closer look later.

I think we should be really careful so that we don't accommodate out-of-tree Kconfig configuration too much in the main repo though. It adds enough complexity that there has to be a significant advantage.

Keeping the main repo simple and understandable is probably more important on the whole than making life easy for people doing out-of-tree stuff. Linux has the right balance there I think.

[ulfalizer]

If someone doesn't mind being inconsistent and wants to indent items within ifs and choices, etc., I'll approve it by the way.

It makes Kconfig much easier to read and less error-prone, especially when you have nested ifs. Shame it wasn't what people did in the beginning. Seen it in some projects at least.

[thomasstenersen]

@ulfalizer Thanks for taking the time to review the changes. I believed I've addressed all the comments with the latest push.

[ulfalizer]

I'm a noob on the Bluetooth stuff, but getting rid of some selects seems nice.

Maybe things could be refactored further so that you don't have to enable lots of different things to be able to turn on NET_L2_BT (worth checking if all combinations of values for those depends on'ed symbols make sense), but that's unrelated.

[ulfalizer]

Maybe this could be called CLOCK_CONTROL_OUT_OF_TREE_DRIVER or the like, to shorten it a bit. It might be clear anyway that there must be an out-of-tree driver if it's on.

@carlescufi suggested something similar. He's more familiar than me with Bluetooth though.

[thomasstenersen]

(@carlescufi) We had a naming discussion here: #13631 (comment)

The reason for the _MUST_USE_ (originally _HAS_) was to keep consistency with other select-able Kconfig symbols. If we keep select-able symbols on the form <noun>_<verb>_<noun> (FOO_HAS_BAR), it is much easier to keep your sanity when working with the Kconfig files.

The name is verbose, but I'd rather sacrifice verbosity for clarity.

[carlescufi]

If you really want to enforce the <noun>_<verb>_<noun> (which I don't think we could call a convention right now except for the _HAS_ case) then I would go for something like *_SELECT_OOT or REQUIRE_OOT something like that. MUST_USE sounds convoluted and verbose to me.

[SebastianBoe]

The original proposal of _HAS_ didn't make sense, but now I'm unsure if we actually considered using an option without HAS. As Ulf says, if the OOT driver is enabled, that does imply that the other drivers can't be. The symbol for an enabled out-of-tree driver would look like:

CLOCK_CONTROL_OUT_OF_TREE_DRIVER

[nashif]

hmmm, why do we need to have Kconfigs for this? Are we going to have such a kconfig for each driver subsystem? Are we talking about 2 conflicting drivers and we would select the one out of tree instead of the one in the tree? In short, this looks like a hack to me. Need some documentation or a design proposal explaining how this can be done in a generic way without polluting Kconfig with such options.

[SebastianBoe]

Need some documentation or a design proposal explaining how this can be done in a generic way without polluting Kconfig with such options.

Please prove me wrong @ulfalizer; it is unavoidable to pollute the kconfig with these options without resolving #8181, which is not going to happen any time soon.

For your other questions, please see the commit message in 8f1e6cb

[carlescufi]

The reasoning is described in this commit message: 8f1e6cb, but I think @thomasstenersen might want to update the PR description with a clear definition of the problem and why these additional Kconfig options are required.

[thomasstenersen]

Edit: hadn't refreshed the page before commenting. I can copy the following comment to the PR description. It provides an answer to what would be my initial reaction to seeing this PR as well :)

Yes we need this, unfortunately. This is for the case were an out-of-tree component for some reason must use it's own driver. There is no way in Kconfig that I and others have found to effectively de-select a Kconfig choice/config without severely polluting the Zephyr configuration files.

Are we going to have such a kconfig for each driver subsystem?

I would add it at a per use-case/need basis. But this is a generic pattern, yes.

Are we talking about 2 conflicting drivers and we would select the one out of tree instead of the one in the tree?

Yes. And the fact that they are not only conflicting but another subsystem must use the out-of-tree one. We also want this switch to happen automatically, so if I select a different BLE controller in the menuconfig, the correct driver dependencies are resolved without the need for other changes by the user.

In short, this looks like a hack to me.

I would like it for there to be a nicer way of doing this. But as far as I can tell, this is the most pragmatic, least intrusive and nicest way of handling out-of-tree dependencies like this. IMO the only way of actually solving the root problem is to resolve #8181.

Need some documentation or a design proposal [...]

What would such a proposal look like? Could I add more to 8f1e6cb to explain the why and how better?

[carlescufi]

@aescolar

That main issue being how do we get out of tree folders to be part of what cmake and Kconfig go thru (similar to the BOARD/SOR/ARCH_ROOT options), but for any arbitrary set of component.

As @SebastianBoe already mentioned, this is already possible with or without west. You can either use west to list the modules you need or you can provide the build system with a list of them directly via the ZEPHYR_MODULES variable: https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/zephyr_module.cmake#L11

[thomasstenersen]

Closing PR to rework changes to be more Nordic specific.
I'll create two new PRs. One for the various minor fixups and one for the out-of-tree driver support itself.