-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Add device init/de-init functionality #84394
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
Add device init/de-init functionality #84394
Conversation
Arduino is extremely interested in this functionality as well, so thanks for providing a quite comprehensive starting point for discussion! 🙇 |
Would this allow for in some manner dynamically loaded drivers using llext? |
I'm not very familiar with llext, but I assume that would require more work to have dynamically loadable drivers. |
What is expected in the We are clearing overlapping things with power management and we need to have better integration Now, how to handle Obviously |
This would be great for CircuitPython as well! My expectation is that de-init would de-init the device and any dependencies that are no longer in use, such as clocks. We do "in use" tracking in CircuitPython and it is very helpful when a vendor SDK handles it too (ESP-IDF 5 does). |
Thanks for the proposal @gmarull. Some thoughts: Note that I expect many users will not need this feature (they will continue with just init'ing once at boot), but will dislike the churn of yet another API change, or increased RAM/code use.
In general no. I expect most users will want to continuing init'ing at boot. Anyhow, from the use cases you list, I'm not specially clear about what one should expect from such a device driver deinit:
I guess a user of the driver may want to imply "I really need this device to release the resources for something else", or "I don't need this by now", or "I don't need this ever again". About naming: device_reserve(), device_release() could sound a bit better to me. |
I'd probably explore the options to keep macros untouched, but
I see this is a core feature, so we need to be careful if made optional. If we e.g. make
I think it's not even doable right now. But once Zephyr transitions to
No, de-init is not directly related to low-power states. A device may just enter idle/lp state by turning off its clock. It's true that by de-initing a device, system will likely consume less power, but that is not the goal of this API.
Yes, it must release any obtained resources (e.g. a clock, a DMA channel, etc.)
Yes, leave peripheral as in reset state. Some MCUs can easily do that (STM32).
A parameter or extra APIs could be options. I thought about this, if we integrate PM functionality into the device model itself.
|
Leave device in its reset state, also releasing any obtained resources.
I agree, this opens the door to de-duplicate some efforts.
Maybe for
Note that here, device_put would end up de-initializing a device, not suspending it. As noted in the RFC, we could change this by making |
Regarding the naming: device_open/device_close seems to be more obvious than device_get/device_put. |
Notes for arch meeting:
|
Architecture WG:
|
It'd be very tempting to suggest the this could actually replace PM runtime entirely, the init - get - put - deinit looks very much like the whole resume - on - suspend - off in pm runtime/power domains. I understand that there's some differences now but this may be a chance to revisit the whole device states story and come back with a coherent story, and since it'd be a breaking change that'd be a chance of rewriting current pm enabled drivers to stick to the new structure and have some better samples in the code base that then all other driver developers will undoubtedly use as references for all new drivers and one year from now the whole code base is full wonderful perfectly implemented drivers with suspend/resume/on/off support. |
For runtime multi-target switching or runtime pinctrl assignment, aren't we supposed to improve pinctrl? |
deinit is a bit more flexible as it also "releases" resources like clocks, dma channels, shared buffers, theoretically interrupts? etc. With deinit, we have one simple mechanism for "freeing" all resources from the applications perspective, compared to enforcing the application/subsystems to know of and interact with individual resources like pinctrl states directly. |
ac67a36
to
259e51a
Compare
Last push:
What I think should be discussed (next Arch WG):
|
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.
deinit is a bit more flexible as it also "releases" resources like clocks, dma channels, shared buffers, theoretically interrupts? etc. With deinit, we have one simple mechanism for "freeing" all resources from the applications perspective, compared to enforcing the application/subsystems to know of and interact with individual resources like pinctrl states directly.
I feel like we are going in the direction where the devices are hotpluggable like in Linux while the devicetree is still compile time not run time. Although you can free up only some of the resources maybe even very limited but not ram usages.
kernel/device.c
Outdated
return ret; | ||
} | ||
|
||
int z_impl_device_put(const struct device *dev) |
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.
I see that usage_lock have the potential to replace all the individual k_sem in each device drivers. Does this mean we can safely remove all the acquire and release in each API implementation?
return 0; | ||
} | ||
|
||
ret = pinctrl_update_states(uart_config, uart_alt, ARRAY_SIZE(uart_alt)); |
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.
I don't see how device_get/device_put can guarantee the safety of switching pinctrl... Should be checking usage count as zero and then switch pin? Only the device driver will know its usage, particularly with async methods, even with a usage count zero does not guarantee the async operation had completed, the device_put should involve checking the device status with the device driver instead of making assumptions.
Last push: should be the final version. Last 4 commits will be dropped and moved somewhere else, they're still here to illustrate a usecase. |
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.
what is wrong with using section iterable… thought it was neater than having this additional device flag?
@gmarull - would be great to ensure bisectability of Otherwise, LGTM. |
1494cdb
into
zephyrproject-rtos:main
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
In #84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
Jumping in late here since I only just found out about this addition. |
So we have a core API function, other subsystems adding functionality to support it and samples being added before we document what the functions are supposed to do, how drivers should implement them, and how applications should use them? That seems backwards to me, how can we review whether code implements an API if the API isn't documented? If it isn't clear from the start we will just end up with every driver doing its own thing. |
It was merged after multiple review rounds and arch meetings with clear use cases in the description of this PR. This is what its supposed to do: zephyr/include/zephyr/device.h Lines 877 to 894 in 11c543e
|
A quick note here: device_deinit() is heavily unsafe (just see the scription saying |
I'm not suggesting it wasn't discussed, I'm suggesting it is not well documented. What does "release any resources" mean in practice? Does it only mean getting the software back to a state such that the hardware can be re-used for a different purpose (nRF I2C vs SPI for example)? Does it also include hardware state that doesn't have any access control (Should every GPIO be put back into the default state as it was after a SoC reboot)? What does "its status will be left as in its reset state" mean in practice? Does this mean anything beyond the "release any resources" requirement previously listed? Are we explicitly trying to put devices back into the state they were on a power up (even if that is a worse idling state)?
Usage being application-specific is fine, but if the API is hand-wavy, there is a question as to what exactly PM for example should be doing in response. This is mostly in response to the above points. And if the answer is that what "de-init" means is specific to individual drivers, where can drivers document what their particular implementation does (and the API wrapper should point users to that)? |
I think the spec is clear enough: release any taken resources and leave peripheral in its reset state. In some platforms, this may have some unforeseen side effects, but well, that's what happens when trying to abstract something that is difficult (or impossible) to abstract. I'd say it's not a problem of this specific interface, though, some other Zephyr APIs suffer from the same (just see the reboot API). |
In zephyrproject-rtos#84394, `struct init_entry` was modified to remove an unneeded union. The `SYS_INIT_NAMED()` macro was adjusted accordingly, but is no longer C++ compatible due to the partial designated initializer. Add an explicit value (NULL) for the other field (`dev`) in that struct. Signed-off-by: Tristan Honscheid <honscheid@google.com>
This RFC is about adding a new core feature to the device model: device
de-initialization.
Why ?
One may think that this is an unnecessary feature since Zephyr primary goal is
to allocate resources at compile time. However, cases arise occasionally asking
for more dynamic features, see e.g. #20012, #40385.
The following scenarios would benefit from being able to de-initialize a device:
(1) Multi-functional IP (e.g. USART/SPI/I2C)
(2) Re-allocation of device resources (e.g. pin re-assignment)
(3) Runtime interpreters like Micropython (where some devices may be dynamically loaded based on user input)
(4) Bootloaders: to leave some devices in their reset state
(5) See concrete case: "Allow multiple ICE40 on the same SPI bus" #77980, #80010
One could argue that all these cases could be solved by exposing custom device
level APIs, however, since this requirement applies to a few vendors, solving
the problem in a more generic way seems like a better approach. Some may also
argue that PM can partly solve some of these problems, however, we should
acknowledge that Zephyr PM subsystem has not been widely adopted, and still
today, many of its design points are discussed #84049.
How... ?
To support de-initialization,
device_deinit()
is proposed. However, while all devices now have an init function, they donot have any de-init function, so the following is introduced:
And introduce additional device definition macros that allow passing a de-init function, e.g.
DEVICE_DT_DEINIT_DEFINE(node_id, init_fn, deinit_fn, ...)
.Open questions
All have been discussed. The Agreement was to implement the simplest option: a new deinit API with no guarantees, no usage tracking, etc.
Samples
[DNM]
show required driver changes or sample de-init implementations