Skip to content

Refining Zephyr's Device Driver Model #6293

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

Closed
tbursztyka opened this issue Feb 21, 2018 · 8 comments
Closed

Refining Zephyr's Device Driver Model #6293

tbursztyka opened this issue Feb 21, 2018 · 8 comments

Comments

@tbursztyka
Copy link
Collaborator

tbursztyka commented Feb 21, 2018

For more information on Zephyr's Device Driver Mode, refer to http://docs.zephyrproject.org/devices/drivers/drivers.html

Current situation

  • A generic struct device with a generic struct device_config
  • Shared system with services initialization (init.h), device and service initialization are interleaved (levels/prio).
  • Optionally, power management generic functions are present (in the config part)
  • Each device is identified by a "name" (stored in the config part)
  • Such name is "maintained" through 4 possibilities (Kconfig, dts, macro or hard-coded)
  • The name is only used to get the device bindings at runtime
  • A device is uniquely allocated in device driver code through DEVICE_DEFINE() or DEVICE_AND_API_INIT() or DEVICE_INIT(). It requires as many calls of one of these macros as there are devices then.
  • device's variable name is not generated from the above name (you cannot un-stringify on preprocessor level), thus it requires a hard-coded unique and un-quoted name.
  • Each device are static but not constant (same for their config part)
  • Specific device settings can be set either via Kconfig, dts, macros or the 3 of them.
  • Most of these settings are specific to each device driver (no generic way)
  • No error reporting when device initialization fails besides local debugging
  • IRQ connection are made at runtime

There are good reasons for all of it, mostly. When ramping up Zephyr, there was not dts yet, Kconfig was the only way.

All of it works just fine. But as we see more architectures, more SoCs, boards and thus more devices being ported: it starts to clutter the system a bit and can be somewhat confusing to new comers. Moreover when things could be simpler and better optimized.

This is a known problem, on-going work is already fixing some of it. Let's list issues/ideas on how to improve the Device Drivel Model.

Tooling

  1. Refactor/rewrite extract_dts_includes.py
    Issue: Cleaning up a bit dts scripts #6504 But there is still more to do

  2. Drop dtc C based compiler for the python version (PR Python replacement for DTC [DNM/WIP] #5615), this would help for the below changes probably (and maintaining current dtc for windows gives extra work)

Simplifications (S)

  1. Move all boards (arch/soc/devices) to dts. Issue: All boards for all arch's should use DTS #4960

  2. Remove Kconfig device specific settings and use dts only
    Only generic support would survive (CONFIG_GPIO, CONFIG_IEEE802154, etc...) ?

  3. DEVICE_* macro call for each device should be externalized from device source code:
    The idea would be to generate these automatically from dts.

Optimizations (O)

  1. Reversing the actual device and config interdependency to gain a bit of space:
    SYS_INIT would then take less space than currently (ok just 12 bytes less)

  2. Remove the 'name' attribute in struct device_init (aka device_config):

    This will need many steps, but mostly requires S1 to be done in order to avoid messing up with Kconfig. I already tried quickly to hack around dts, and Kconfig, since not all boards are dts ready and it is very ugly. It was just a quick PoC. (see https://github.com/tbursztyka/zephyr/tree/dev_name_const).

    Zephyr will gain some ROM space from it: no device name, and direct bindings (which also will improve boot time a tiny bit)

  3. Remove drv_name in DEVICE_* macro, keep only dev_name (which is the semantically right name, there is no such thing à as "driver name" anywhere at runtime).

  4. Make all devices constant:
    Easy now that getting the bindings are made at built time. (const in DEVICE_ macro, extern const struct device ... in the generated file etc...)

  5. DTS can generate such unique name via node name, thus 'label' could be removed altogether. No need for any name maintenance then in dtsi/yaml files.

  6. Revise how IRQ are connected: this would reduce runtime necessary calls for it and save some ROM space, while simplifying maintenance as well (connection could be generated automatically via the dts workflow)

  7. Do not built/link built device drivers, and there relevant device instances, if none of these are used anywhere. I.e. even if gpio port C is requested to be built, but proven to be let alone then: do not embed it in the final image. Again saving ROM space relevantly.

Issues ordering

  1. S1, O2 in parallel (S1 is anyway going to take quite sometime)
  2. S4
  3. O3, O7
  4. O4
  5. O5, O6 in parallel
  6. O8

Let's discuss and update this issue accordingly

@erwango
Copy link
Member

erwango commented Feb 21, 2018

Hi @tbursztyka ,

Thanks for structuring this topic and giving some directions.
There is already some work under RFC to address some of the items you listed. Let me list here what I 'm aware of (mainly since I worked on it).

dts based pinmux generation

#5021: Pinmux configuration code generated from Device tree
#5799: drivers: pinctrl device interface (Linux-like) and STM32F0 driver

These 2 works are concurrent and don't cover exact same scope but both provide a way to generate c code for pinmux board definition.
#5021 is purely python based, use existing pinmux driver and extract_dts_include script to generate a pinmux table located in a include/generated file that could be used by pinmux driver based on dts pinmux nodes. In that sense, it is close to point S4 you define
#5799 proposes a new pinctrl API (based on current pinctrl API) and uses code generation library (chaos). It is a comprehensive work that address several topics. It requires to be split to discuss each point separately but is for sure quite interesting to look at. It provides a way to generate pinctrl driver initalization code. Some work is still ongoing to minimize the macro "burden".

Driver init function generation

Available in the following branch:
https://github.com/erwango/zephyr/tree/dts_struct_pinctrl
Main idea is that it provides a generic method to generate driver init code(applied on UART), per instance, in a <outdir>/include/generated/<driver_compatible>.h file. It is one way to implement your point S4.

Learnings so far

All these works have flaws, and should be refined. Main learnings as of now from the feedbacks:
-extract_dts_include deserves a rework, to be more easily maintained (use of class, methods). This is started to be addressed in #5799
-Code generation requires all boards to use same models and APIs. For instance, there is too much variations in pinmux across existing arch/boards to provide a code generation solution that could work for all. On that part also, #5799 is interesting as it proposes a pinctrl API that could cover all arches.
-Most of the generation should be transparent to the user/driver developer. In that sense, in don't think #5799 could be widely deployed as of now.

@tbursztyka
Copy link
Collaborator Author

I noticed indeed that we are many chasing similar rabbits, so it would be nice to centralize efforts. A bit like what is being done for the APIs in #5697

extract_dts_include would require some love indeed, even renaming it would be wise as it's going to be used for more various things. it's quite messy as it is. (kconfiglib.py for instance is super easy to manipulate, it's well commented etc... )

About the #5799, I'll take a look (the pr is big), but importing yet another external tool (chaos_pp), not sure it's wise. Moreover your dts_struct_pinctrl branch is playing with yaml/dts nicely to do what chaos_pp has been kind of used for in #5799 it seems, but your solution is generic and fits better in S4.
I'll comment there.

About your branch, it seems to me that it would be the right approach. There are technical details that may change. For instance: is it better to generate a header that is then included in the driver, or generate one .c file instantiating all devices (knowing that the same routine would generate the future header listing all devices as extern as described in O3). Also, maybe using some template for generic part (DEVICE_* macro usage) could make the code a bit clearer. We need to find a way to manage specific irq settings as well. Some devices need more than 1 (spi_dw on arc for instance), or need to request to unmasq these and so on. Finding a way to solve this in a generic way could be a little bit challenging.

But first things first: we need to finalize porting all arch/soc/boards/devs to dts. Maybe it would be useful to list what's missing (not sure there is an issue listing what needed to be done on that front at least). There are even devices drivers I made that miss such support so... :)

@lpereira
Copy link
Collaborator

By moving more ports to DeviceTree, it should be possible to revisit IRQ_CONNECT() as well. The current best known method to "connect" an IRQ line to an ISR is quite awkward, and it would be good if we could do as much as possible during compilation time, without requiring a "call" to IRQ_CONNECT() to be inside a function. This is technically possible already, given that most ports are using the gen_isr_table script.

@cvinayak
Copy link
Collaborator

@lpereira first, very good issue (descriptions).

For me, contributing a bare-metal BLE controller into Zephyr and slowly porting to re-use Zephyr OS features, every change gets me to profile the controller with respect to CPU usage (current consumption). Most BLE end-user usecases are race-to-idle, which means CPU wakeup is always by IRQ then followed up by (seldom) thread mode execution, and of course, in all such wakeup there is the use of Driver API.

Observation from the current driver API is (without care to use const), each API call adds added latency passing device structure in RAM, indirect memory access needing many instruction cycles in an MCU.

Please consider these API call overhead in this issue addressing refinement of Driver Model (if not considered already).

Reference:

@cvinayak cvinayak reopened this Feb 22, 2018
@b0661
Copy link
Collaborator

b0661 commented Feb 22, 2018

Some (biased) remarks from the author of #5799 regarding

DTS extraction and driver data/code generation

I see #5799 (green) and #5021 (yellow) as two of several options a driver implementer may choose from. She may also decide to write her own script to transfer DTS info to the driver. All this should be controlled by the CMakefile of the driver inplementation.

device driver thoughts

The Zephyr device driver infrastructure should be open to several solutions to enable the driver implementor to select the one that best fits to the needs. The device driver infrastructure should provide capabilities such as

  • DTS data in several formats (generated_dts_board.h, generated_driver_data, xml, json, ..),
  • Device function library for device init/ setup/ ...,
  • Driver template expansion support,
  • Hooks for device driver generator scripts.

No specific solution should be mandated.

The yellow solution for example seems to be less complex and fully sufficent for an e.g. adc driver. It will most probably not work for a pinctrl driver. It may be too inflexible in case not only data but also template functions have to be instantiated for a device (e.g. to call IRQ_CONNECT).

@tbursztyka
Copy link
Collaborator Author

tbursztyka commented Feb 23, 2018

@b0661 I have to disagree on "No specific solution should be mandated."

The least freedom you'll give in this domain will lead to a bunch of completely different ways of doing things. And who will maintain what then?

The idea of having one solution fits all, is that everybody will "speak the same language". Anybody should be able to go and fix easily any driver, board, device integration, review PRs, etc...

I have a preference on the yellow approach mostly because it does all in python in a tool zephyr owns fully (no need of any external tool) and which removes details of device instantiation in driver code.
True, it's not flexible enough, yet. Many devices have a specific way of configuring interruptions, or getting there specific configuration bits and pieces together. That's exactly why this issue has to be brought up, so we find one way to solve all these specific cases in a nice manner.

@lpereira Sure, I'll revisit/rewrite the issue and add that one. I need also to add the one dealing with avoiding linking unused device: currently, even if a device is not used by any part of the code, fact that it gets its driver and its instance brought up will make it part of the final image.

[edit]: issue updated. Btw: is there an issue tracking what's being done moving towards DTS (S1)? Could not find any.

@SebastianBoe SebastianBoe changed the title Refining Zephyr's Device Drivel Model Refining Zephyr's Device Driver Model Feb 23, 2018
@b0661
Copy link
Collaborator

b0661 commented Feb 23, 2018

@tbursztyka

I have to disagree on "No specific solution should be mandated.". The least freedom you'll give in this domain will lead to a bunch of completely different ways of doing things. And who will maintain what then?

No specific solution should be mandated means: even if there is a preferred solution other solutions should be at least supported by the technical capabilities (e.g. hooks, data formats, ...). The capabilities shall be available to allow new solutions to emerge. That does not mean that there is a bunch of different solutions used. The solution(s) to be used should be decided by the review process and not dictated by technical shortcomings.

The idea of having one solution fits all, is that everybody will "speak the same language". Anybody should be able to go and fix easily any driver, board, device integration, review PRs, etc..

I agree. The point to discuss is the features of the "language".

python ... a tool zephyr owns fully (no need of any external tool)

In this sense CHAOS is not a tool but a set of macros comparable to python scripts. The respective tool is also fully owned by Zephyr - it is the C preprocessor. I think the CHAOS thing is somehow overrated. A big deal of data preparation is done in extract_dts_includes.py. generated_dts_board.h acts as a 'database' with a strange data format - C defines. The CHAOS C preprocessor macro library perfectly fits to this data format. There is a work split between extract_dts_includes.py and some C preprocessor macros.

I have a preference on the yellow approach mostly because it does all in python ...

As described above this is a matter of work split between the python tool and the C preprocessor.
The more you do in python the less has to be done by the C preprocessor. It is also a matter of flexibility and freedom.

If everything is done by the python tool there is no flexibility and freedom to do something different if it is needed. At that point you start providing templates, configuration data, ... to the python tool or change it. In the end you come up with some "language" to specify the driver type to be generated by the tool.

On the other side if you hand over less eleborated data but provide eleborated "language" contructs for the C preprocessor you create the driver configuration "language" on the C preprocessor side. Doing that you do not have to foresee future needs of the driver implementers. You just provide means to empower them to do what is necessary.

... removes details of device instantiation in driver code.

This can be achieved independent from the used tooling or work split. Device instantiation is already handled by macros and may be handled by C code generated by python too.

True, it's not flexible enough, yet. Many devices have a specific way of configuring interruptions, or getting there specific configuration bits and pieces together. That's exactly why this issue has to be brought up, so we find one way to solve all these specific cases in a nice manner.

So my suggestion is: use the C preprocessor and some sophisticated macros to get that flexibility. I already created template like interrupt configuration for the STM32 SPI driver in #5799.

Thx.
Bobby

@tbursztyka
Copy link
Collaborator Author

I am closing that one, since issue #22941 is meant to be a more exhaustive replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests