Refining Zephyr's Device Driver Model - Take II

[tbursztyka]

(This issue is a replacement of #6293 )

A deep revise and upgrade of the device driver model

Throughout its short existence, Zephyr has seen its kernel reworked, stabilized and made reliable with sound and concise APIs and objects. All still being designed within the scope of running on tiny targets, such as arduino_zero with 32k of RAM or bbc_microbit with its 16k of RAM for instance, but also providing enhanced features for fully fledged industrial boards with much more memory and CPU power.

Next to the kernel, the device driver ecosystem has experienced somewhat a different development.
The device driver APIs have been improved to cover most of identified generic use-cases, and are still being reworked, sometime deeply refined to adress new requirements. Surprisingly however, the core of this ecosystem: the device driver model, has gained little to no improvement at all. It does not make it void of issues, far from it actually. In fact, some of the issues, known from the beginning sometime, have been a drawback for implementing advanced features. Worse, as time goes on with the device driver ecosystem growing everyday, these issues will tend to be more and more tedious to get fixed. It is not to say there are no attempts to fix some the known issues, there are. But I believe they are not generic nor concise enough, though some are definitely interesting test beds.

Hence a list of the known issues and a proposal to address these.

Identified issues

• (1) Power Management is broken:
  The lack of dependency tree support among devices makes it unusable. The burden of managing client devices and their parents within the power management context is pushed to the user which has been proven to be tedious, unreliable and thus made the PM subsystem nearly - if not fully - unused at all. Due to this most device drivers do not implement the PM API.

• (2) No support for cancelling on-going device API call:
  In PM context or else, it might be relevant to cancel an on-going call on a device. Stopping and/or re-initializing a user thread would require such feature for instance. A caller could also decide to cancel its own calls. See [RFC] Device: generalizing and improving control features (sync, lock and timeout) #24511

• (3) No support for a timeout on each and every device API call:
  Being generic to all device driver API such feature is one of the important missing. It may happen, either because the system is overloaded or because the hw is not responding, that a call will never come to a closure. Thus locking up the device on the call as well as the caller waiting for something to happen. This prevents also the caller having the possibility to mitigate any error in case the call would be unsuccessful. See [RFC] Device: generalizing and improving control features (sync, lock and timeout) #24511

• (4) No error storage when device initialization fails or gets into hw fault:
  In case of initialization failure, the device will just be inaccessible, and there would be no way to know what happened. In case of a run-time, non-blocking, hardware fault, only the user will get an error back. Localized logging cannot be sufficient in this case. See [RFC] Device status reporting scheme #23589

• (5) No possibility to recover a device in case of initialization failure or hw fault:
  Such failure could be due to boot timing, temporary hw issue, cosmic rays, gremlins, etc... Currently, a device failing the initialization step will not be usable at all afterwards. One can decide to reboot, but if the boot sequence is the culprit, the device will never be recoverable. It should be possible to call init again. Either to fix such booting issue but also at run-time where recovering a device may require to reinitialize it (after a locking hw fault for instance). See [RFC] Device status reporting scheme #23589

• (6) Device instantiation
  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. The link with DTS is non-existent unless the developer makes it happen using DTS generated config options, which is the current process.

• (7) Each device is not constant (same for their config part)
  It was meant to be constant, but due to a hack on device's API setting (in case of init failure it is set to NULL at runtime) it is never constant, thus ends into RAM instead of staying into the ROM. See [RFC] API change: Device - Const-ify device driver instances. #24873

• (8) The device name:
  It is only used to get the device bindings at runtime and thus reduce boot performances and takes ROM/RAM for nothing.
  devices: make device name field optional #49352

• (9) Nameless device created by SYS_INIT():
  It does not make any use of the name, API, PM API and thus use too much memory for nothing. See Refactor device structures #23407

• (10) device_pm structure
  An obvious case of under optimized structure

• (11) Device's variable name
  Is not generated from the device name (you cannot un-stringify on per-processor level),
  thus requiring a hard-coded unique and un-quoted name. That issue is due to the allocation macros (see issue 6).

Other know issues already recorded:

• (12) Device initialization order that respects device tree dependencies Device initialization order that respects devicetree dependencies #22545
  Related prep-work: init: specify dependencies between init entries #49318

• (13) Adding support for multi-functional-devices Adding support for multi-functional-devices #48934

Solutions

(Follow the same order as the issue listing)

1. PM dependency tree

   (issue 6 needs to be fixed first)

   DTS, as its name tells, knows every devices and their parent/child that will exist in the build for a particular board/SoC. Thus it makes fully sense to use this information to create the PM dependency tree. For instance, suspending SPI master device x will first suspend all its related SPI devices k, n and m before actually suspending x.

   From a technical perspective, it's just an oriented graph. Thus all well understood algorithm managing it. From an implementation point of view however, it's very important to keep in mind such graph could be greedy in memory depending on how many devices are present, and more importantly, how many relationships exist among them obviously.

   Hence, a proposal here that could help to keep a tiny memory footprint:

   struct device_pm {
       ...
       const u16_t *childs;
   };
   

   Where such child array would be, for the SPI device x:

   const u16_t device_x_childs[] = [ device_k, device_n, device_m, device_end ];
   

   Note that device_<k/n/m/end> are not pointers, but the result of a simple operation:

   device_<k/n/m> = dev_<k/n/m>_pointer - __device_init_start;
   

   and device_end would be:

   device_end = __device_init_end - __device_init_start;
   

   That one would be used internally to know where the array stops.

   All of it would be generated by DTS and the linker would do the rest. It takes twice less space compared to full pointers.

   A completely separate tree could also be created, not in device_pm structure as seen above. It would result in more CPU usage in tree traversal to retrieve if a device being suspended or resumed is in there as a child or a parent. However, note that any accessed device will require anyway to retrieve its potential parent(s). So as a device tree is unlikely going to be thousands of nodes anyway, such traversal could be fast enough. It's worth studying what is the best of all solutions here.

   Storing the actual DTS tree itself in the binary is not the right path: it would take much more memory and anyway would require as much CPU power than above, if not more due to comparisons being made on strings.

   I believe the node id size, u16_t here, could be generated by DTS as well depending on the amount of devices, it could even go down to u8_t. But that would mean DTS knows about the boot services too, which are actually made of struct device and thus part of the same device array. It is not the case at the moment and impedes fixing issue 12 as well.

   As noted in the introduction, some existing work could be reused to fix this issue. For instance, the on/off API is close to this PM requirements and thus its core could be partly back-ported in PM but improved for various state and not only on or off. Mostly the logic of waiting for all nodes to switch from on to off and conversely could be reused for PM state switch among all nodes.

2. Cancelling on-going device API call

   Adding a generic device API function such as device_cancel_call(struct device *dev) could do the job. Would there be a necessity to check on caller's thread id, it would require to store such id on each call, perhaps. In any case, it would require device structure to hold a pointer on a device driver specific function to actually properly cancel a call and cleanup the hw (reinitialize it if needed etc...).

3. Adding a timeout on each and every device API call

   Such timeout would be a generic attribute to all devices, so added into struct device. The non trivial part is how to populate the timeout and with what value. Such timeout value could be a fixed one, provided via Kconfig and/or DTS. But it could also be computed at run-time for the bus specific API for instance where it is possible to evaluate rather precisely how much time a transaction would take. Based on the frequency, word size, length of the whole transaction etc... For the API providing a timeout directly, the value would be then then one given via such API. Or more specifically, the default value would be superseded by such value.

   Perhaps such feature could be made optional, via Kconfig, for tiny targets.

4. Error storage when device initialization fails or gets into hw fault

   Such initialization failure needs to be stored, but also when a hw fault arises. These are separate errors and thus should have there own storage space. One bit for each would be sufficient, so a bit field would go. It might be worth to store the error as well (either limited to 8 or 16 bits?).

   It could be worth studying if the PM busy bit could be stored there as well, unless we generalize a lock on each and every device, reducing many device driver's internal data storage as well as generalsing lock mechanism for concurrent access. The PM state could also be stored in the same bit-field to simplify things.

   Usefulness of such error/state storage would be:

   • to re-initialize a device in case of boot failure and/or hw fault
   • in case of hw fault, any subsequent calls would not proceed and require to re-initialize properly the device. (avoiding cascading error calls)
   • a base for reporting such error to any type of use-case requiring fine grained device state reporting (for maintenance, etc...)

   Again, such feature could be made optional, via Kconfig, for tiny targets.

5. Recovering a device in case of initialization failure or hw fault

   As soon as issue 4 is fixed, it would be trivial to relevantly call device's init function based on the stored error/state.

6. Device instantiation

   This is by far the biggest and tedious feature. As noticed earlier, this would pave the way to fix most of the other important issues and would finalize the connection between DTS and the code by letting DTS auto generating everything orbiting around the device concept. Also noticed earlier, prototypes have been undertaken already but were too complex from there usage point of view. However on PR scripts: inline code generation with cogeno (and edts) #10885, one can see some examples, and it reaches a point where developer would have very few thing to do on device driver side.

   But it could be made even simpler. Something like:

   /**
    * @code{.codegen}
    *
    * device_declare(<yaml compatible name>, <init prio_flag>, <level>, <irq_func>, <init_func>,
    *                           <exposed api>, <internal data_struct>, <internal config_struct>)
    *
    * @endcode{.codegen}
    */
   

   Note that and could be removed once issue 12 would be undertaken. DTS would know that information, and thus would not require to get it from such device_declare() pseudo-code.

7. struct device and config part made constant:

   That is trivial once issue 4 is fixed as the api pointer would not be tempered with anymore. A coccinel script could help to check the whole code-base for compliance to this rule as well.

8. Getting rid of the device name:

   That one is actually very easy, once issue 6 is fixed. Again, DTS could generate a header file such as generated_devices.h which would enumerate each and every device instance such as:

   #include <device.>
   
   extern const struct device *device_k;
   extern const struct device *device_n;
   extern const struct device *device_m;
   ...
   

   And the (infamous) device_get_binding() would just be a macro, replacing the "call" to device_get_binding(DT_LABEL_DEVICE_K) into device_k. It's in fact very trivial. A previous q&d patch I did back in the days was able to show off this feature, however not based on DTS at that time (it got barely introduced then) but on Kconfig. And it worked very well, the gain of ROM/RAM was interesting and the boot time was better as well, since device_get_binding() was not anymore retrieving a device by its name.

9. Nameless device created by SYS_INIT():

   A simple struct device, and its components, refactoring would be sufficient. Again, that was already prototyped back in the days for the issue 8.

10. Optimizing device_pm structure:

• the dev attribute can be removed (CONTAINER_OF() would just work)
• enable and fsm_state could be just in same bit field and generalized (instead of book keeping "internal power state")
• the lock could be generalized in sturct device, and used also for all calls for concurrent access.
• work, event and signal usage should be revised. See on/off API for instance

11. Device's variable name

Once issue 6 fixed, that would also directly fix this issue. See the PR #20253 attempting to generate such unique id "ZID", though the PR goal is wrong as there is no need for making device init and device_get DT aware per-se. Connecting strongly DTS to device (variable name, instances etc...) would anyway make the whole device driver model DT aware. (See issue 6)

12. Device initialization order that respects devicetree dependencies Device initialization order that respects devicetree dependencies #22545

That one would require also first to expose the software services to DTS. That's trivial imo. Then indeed sorting out the priority and levels out of the device tree would require some work on DTS side.
Let's sort the details out directly in relevant issue.

Note that getting issue 6 fixed would greatly help that, in a sense no one would have to go all over the existing device drivers to fix the priority/labels there.

Final thoughts

I believe there is no proper way to fix most of these issues without deeply connecting DTS to the model. And as such, we should focus on fixing issue 6 (aka "instanciating device via DTS code generation")

[carlescufi]

CC @galak @pabigot @nordic-krch @erwango @MaureenHelm @nashif @anangl @henrikbrixandersen @mnkp @mbolivar

[andrewboie]

One more known (but relatively minor) issue that affects kernel footprint:

The model of placing function pointers in an API struct defeats gc-sections, as the functions will always have at least a reference in that API struct even if not referenced elsewhere. So if a driver is built, all of its API members will be pulled into the binary (and whatever they transitively pull in) even none of them, or only a subset of them, are invoked at runtime.

At the moment I have no proposals to remedy this, but thought it worth noting.

[pizi-nordic]

@tbursztyka: For Power Management you will need more complex dependency tracking, as you have several types of relation between devices. Examples:

• Parent/Child: Which controller handles given device?
• Device/GPIO: Which GPIO is used to select SPI Device? Which controller controls this GPIO?
• Device/Clock: What provides clocks for the device?
• Device/Power: What provides power for the device?

[nordic-krch]

@tbursztyka thank you for taking that initiative. Few comments:

I would add following requirements/topics:

1. using devices with generic API's Ability to use an API with device without extending its own API #22415
2. device uninit
   One use case is bootloader which should provide clean state before jumping into application. Second use case is nordic specific: we have couple of peripherals which are sharing same hw and address space thus cannot be used at once (e.g. SPI0 and I2C0) and currently in zephyr there is no way to use both in same application. I guess there might be other cases where user may want to uninit driver not because of PM reason.
3. manual/deferred initialization
   Currently, all devices are initialized before reaching application. However, there might be cases where startup time is vital and application may want to defer initialization of some devices (because they might not be needed). One of the examples might be wake up from brown out reset when we want to do some kind of maintenance (e.g. flush some retained data to flash) and we don't want to have full blown system started.

No support for a timeout on each and every device API call

I assume that this is applies only to synchronous calls.

[tbursztyka]

@andrewboie Indeed, very good point. I wonder if an ld script could check if a device instance is being used, besides its presence in the device init section, and would give a warning about it. Though the variable name of the device pointer would probably be not really user friendly (it could be generated from the ZID, thus an indirect way to relate to DTS and then to the driver it belongs).
But that sounds convoluted, there is probably a better way.

@pizi-nordic Most of this has to be solved in DTS side. The goal is to embed as less information as possible. Note that a node child may have 1+ parents (spi master and gpio in case of a spi device using a gpio cs, for instance.). The hierarchy of nodes would have to be generated by DTS. On DTS side, a clock source would need to expose whether it can be stopped (or under-clocked etc...), if it's fixed: no need to make it as a parent. Dealing with clock gate, I'd prefer the driver to embed that info directly, though I did not think much of that issue. Could be made generic, let's see.

@nordic-krch I still have to take some time on your proposal 1. I am not yet convinced this require a generic point into device. It would need more use case I think. About point 2, yes that's a very good one. On DW IP blocks for instance it may share spi and i2s. It is indeed a common use case. However, we have to come up with a transparent solution, i.e. something that would not require the user to do anything. Partial solution is about the device lock mechanism, in case of 2+ dev sharing the same resource, such lock will have to be unique for these 2+ devs. But then indeed we need to know: "is dev_x initialized?" and "since I just got the lock from dev_2, how to de-init dev_1 and init dev_2?" and "if I just used dev_2 last time, and reuse it again... I don't need to re-initialize it then?" etc... Point 3 seems valid as well, and it is crossing point 2: the way to know if a device has been initialized or not, and thus reacting transparently based on that info.

[lgirdwood]

No support for a timeout on each and every device API call

I assume that this is applies only to synchronous calls.

Can this be explained in more detail.?

The danger here is that you are pushing device driver specific knowledge up the stack and this becomes unmanageable very quickly. i.e. consider a driver API calls that polls on a HW state change. It could poll forever, but it's the responsibility of the driver (and not the core) to manage this timeout (and return -ETIMEOUT or whatever up the stack).

[tbursztyka]

@lgirdwood There is nothing the core has to do in this case. The idea it to bring a common object into struct device so the drivers will be able to use afterwards in a normalized way, instead of letting each and every device driver reinventing the wheel on their own.

[vanti]

Adding a couple of ideas:

1. Expanding from the line of thought from @pizi-nordic, it'd be nice to have a concept of power domains in DTS. On TI SoCs and likely other ones out there, devices are grouped into power domains, and only when all devices in the domain are not needed then the entire domain can be shut off.

2. Another use for "device uninit" is to employ it as a way for the application to say it doesn't intend to use a device for a long time, so the device can be shut off. With several device drivers currently the driver can only assume a device is not needed whenever there is no on-going data transfer, in which case it'd just turn on and off the device around the "transfer" function for example. But if the application wants to do back-to-back transfers, this can result in an unnecessary large overhead. If there were a "device uninit" then the driver would have the option to turn on the device during init and shut it off when "uninit" is called.