Commit c55650c2 authored by Gerard Marull-Paretas's avatar Gerard Marull-Paretas Committed by Fabio Baltieri
Browse files

[RFC] device: devices no longer depend on SYS_INIT

Background
----------

Nowadays, devices rely on the `init.h` (i.e. `SYS_INIT`) infrastructure
to get initialized automatically. The `init.h` infrastructure is a basic
mechanism that allows to register an init function when the system is
initialized (before `main`). It provides with multiple initialization
levels: `PRE_KERNEL_*`, `POST_KERNEL`, etc., and within each level a
numeric priority (0-99). Using this information, each registered init
entry is sorted by the linker so that the Kernel can later iterate over
them in the correct order. This all sounds nice and simple, but when it
comes to devices, this mechanism has proven to be insufficient.

Before starting with the changes proposed in this patch, let's first dig
into the implementation details of the current model. When devices are
defined, using any of the `DEVICE_*DEFINE` macros, they also create an
init entry using the internal `init.h` APIs (see
`Z_DEVICE_INIT_ENTRY_DEFINE`). This entry, stores a pointer to the
device init call and to the device itself. As the reader can imagine,
this implies a coupling between `init.h` and `device.h`.  The only link
between a device and an init entry is the device pointer stored in the
init entry. This allows the Kernel init machinery to call the init
function with the right device pointer. However, there is no direct
relationship between a device and its init function, that is, `struct
device` does not keep the device init function reference. This is not a
problem nowadays, but it could be a problem if features like deferred
initialization or init/de-init have to be implemented. However, in
reality, this is a _secondary_ problem. The most problematic issue we
have today is that devices are mixed with `SYS_INIT` calls. They are all
part of the same init block, and are treated equally. So for example,
one can theoretically have a system where the init sequence can be like:

```
- PRE_KERNEL_1
  - init call 1
  - device 0
  - init call 2
  - init call 3
  - device 1
  - device 2
  - init call 4
- PRE_KERNEL_2
  - init call 5
  - device 3
  ...
```

This is problematic because:

(1) Init calls can depend on devices, but dependency is not tracked
    anywhere. So the user must check that init priorities are correct to
    avoid runtime failures.
(2) Device drivers can have multiple instances, and each instance may
    require a different set of priorities, while `SYS_INIT` calls are
    singletons.
(3) Devices don't likely need so many init priorities (`SMP`?
    `APPLICATION`?)

(1) is particularly important because init calls do not have a specific
purpose. They usage ranges from SoC init code, to system services. So
it's _unpredictable_ what can happen in there. (2) is a tangential
topic, actually not fixed by this patch, even though it helps. (3) is
more of a post-patch cleanup we need.

So, what does this patch propose...?
------------------------------------

**First, this patch is still a HACK, so please, focus on the description
rather than with the implementation**

So it's about providing devices with their own init infrastructure,
minimizing the coupling with init.h. This patch groups devices in a
separate section, but, keeps the same init levels as before. Therefore,
the list above would look like:

```
/* devices */
- PRE_KERNEL_1
  - device 0
  - device 1
  - device 2
- PRE_KERNEL_2
  - device 3
  ...

/* init calls */
- PRE_KERNEL_1
  - init call 1
  - init call 2
  - init call 3
  - init call 4
- PRE_KERNEL_2
  - init call 5
  ...
```

This means that **it is no longer possible** to mix init calls and
devices within the same level. So how does it work now? Within each
level, devices are initialized first, then init calls. It is done this
way because I believe it is init calls who depend on devices and not
vice versa. I may be wrong, and so the whole proposal would need a
rework. So the init order would look like:

```
- PRE_KERNEL_1
  - device 0
  - device 1
  - device 2
  - init call 1
  - init call 2
  - init call 3
  - init call 4
- PRE_KERNEL_2
  - device 3
  - init call 5
  ...
```

Why does this matter for future development?
--------------------------------------------

First, we have devices grouped at least on a level basis. This means
that we could likely start using devicetree ordinals, by reducing the
source of problems to the init level only. We can also re-think device
init levels (probably init levels as well, hey `PRE_KERNEL_1/2`). For
example, there could be pre-kernel and post-kernel devices only. The
other side effect of this change is that `struct device` stores the
device init call, so we can potentially do things like defer device
initialization or implement things like init/de-init. They all come with
their own complexity, of course, but this patch could be a step forward.

Problems
--------

This is a breaking change. Sorry, guys, another one to the list. The API
does not change, so builds should continue to work. However, the system
changes its behavior, so if anyone was relying on mixed init call/device
sequences, things will break. This is why it is important to analyze
lots of use cases before moving forward with this proposal.

Signed-off-by: default avatarGerard Marull-Paretas <gerard@teslabs.com>
parent dac81a20
Loading
Loading
Loading
Loading
+17 −61
Changes for include/zephyr/device.h: 17 added lines, 61 removed lines.
Original line number Diff line number Diff line
@@ -10,7 +10,6 @@
#include <stdint.h>

#include <zephyr/devicetree.h>
#include <zephyr/init.h>
#include <zephyr/linker/sections.h>
#include <zephyr/sys/device_mmio.h>
#include <zephyr/sys/iterable_sections.h>
@@ -321,25 +320,6 @@ typedef int16_t device_handle_t;
#define DEVICE_DECLARE(dev_id)                                                 \
	static const struct device DEVICE_NAME_GET(dev_id)

/**
 * @brief Get a @ref init_entry reference from a devicetree node.
 *
 * @param node_id A devicetree node identifier
 *
 * @return A pointer to the @ref init_entry object created for that node
 */
#define DEVICE_INIT_DT_GET(node_id)                                            \
	(&Z_INIT_ENTRY_NAME(DEVICE_DT_NAME_GET(node_id)))

/**
 * @brief Get a @ref init_entry reference from a device identifier.
 *
 * @param dev_id Device identifier.
 *
 * @return A pointer to the init_entry object created for that device
 */
#define DEVICE_INIT_GET(dev_id) (&Z_INIT_ENTRY_NAME(DEVICE_NAME_GET(dev_id)))

/**
 * @brief Runtime device dynamic structure (in RAM) per driver instance
 *
@@ -386,6 +366,8 @@ struct device {
	struct device_state *state;
	/** Address of the device instance private data */
	void *data;
	/** Initialization function */
	int (*init)(const struct device *dev);
#if defined(CONFIG_DEVICE_DEPS) || defined(__DOXYGEN__)
	/**
	 * Optional pointer to dependencies associated with the device.
@@ -846,17 +828,6 @@ static inline bool z_impl_device_is_ready(const struct device *dev)

#endif /* CONFIG_DEVICE_DEPS */

/**
 * @brief Init sub-priority of the device
 *
 * The sub-priority is defined by the devicetree ordinal, which ensures that
 * multiple drivers running at the same priority level run in an order that
 * respects the devicetree dependencies.
 */
#define Z_DEVICE_INIT_SUB_PRIO(node_id)                                        \
	COND_CODE_1(DT_NODE_EXISTS(node_id),                                   \
		    (DT_DEP_ORD_STR_SORTABLE(node_id)), (0))

/**
 * @brief Maximum device name length.
 *
@@ -878,6 +849,7 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 * @brief Initializer for @ref device.
 *
 * @param name_ Name of the device.
 * @param init_ Device init function.
 * @param pm_ Reference to @ref pm_device (optional).
 * @param data_ Reference to device data.
 * @param config_ Reference to device config.
@@ -885,9 +857,10 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 * @param state_ Reference to device state.
 * @param deps_ Reference to device dependencies.
 */
#define Z_DEVICE_INIT(name_, pm_, data_, config_, api_, state_, deps_)         \
#define Z_DEVICE_INIT(name_, init_, pm_, data_, config_, api_, state_, deps_)  \
	{                                                                      \
		.name = name_,                                                 \
		.init = init_,                                                 \
		.config = (config_),                                           \
		.api = (api_),                                                 \
		.state = (state_),                                             \
@@ -902,8 +875,10 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 * @param level Initialization level
 * @param prio Initialization priority
 */
#define Z_DEVICE_SECTION_NAME(level, prio)                                     \
	_CONCAT(INIT_LEVEL_ORD(level), _##prio)
#define Z_DEVICE_SECTION_NAME(node_id, level, prio)                            \
	_CONCAT(level##_##prio##_,                                             \
		COND_CODE_1(DT_NODE_EXISTS(node_id),                           \
			    (DT_DEP_ORD_STR_SORTABLE(node_id)), (0)))

/**
 * @brief Define a @ref device
@@ -912,6 +887,7 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 * software device).
 * @param dev_id Device identifier (used to name the defined @ref device).
 * @param name Name of the device.
 * @param init_fn Device init function.
 * @param pm Reference to @ref pm_device associated with the device.
 * (optional).
 * @param data Reference to device data.
@@ -921,32 +897,13 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 * @param api Reference to device API.
 * @param ... Optional dependencies, manually specified.
 */
#define Z_DEVICE_BASE_DEFINE(node_id, dev_id, name, pm, data, config, level,   \
			     prio, api, state, deps)                           \
#define Z_DEVICE_BASE_DEFINE(node_id, dev_id, name, init_fn, pm, data, config, \
			     level, prio, api, state, deps)                    \
	COND_CODE_1(DT_NODE_EXISTS(node_id), (), (static))                     \
	const STRUCT_SECTION_ITERABLE_NAMED(device,                            \
		Z_DEVICE_SECTION_NAME(level, prio),                            \
		Z_DEVICE_SECTION_NAME(node_id, level, prio),                   \
		DEVICE_NAME_GET(dev_id)) =                                     \
		Z_DEVICE_INIT(name, pm, data, config, api, state, deps)

/**
 * @brief Define the init entry for a device.
 *
 * @param node_id Devicetree node id for the device (DT_INVALID_NODE if a
 * software device).
 * @param dev_id Device identifier.
 * @param init_fn_ Device init function.
 * @param level Initialization level.
 * @param prio Initialization priority.
 */
#define Z_DEVICE_INIT_ENTRY_DEFINE(node_id, dev_id, init_fn_, level, prio)     \
	static const Z_DECL_ALIGN(struct init_entry) __used __noasan           \
		Z_INIT_ENTRY_SECTION(level, prio,                              \
				     Z_DEVICE_INIT_SUB_PRIO(node_id))          \
		Z_INIT_ENTRY_NAME(DEVICE_NAME_GET(dev_id)) = {                 \
			.init_fn = {.dev = (init_fn_)},                        \
			.dev = &DEVICE_NAME_GET(dev_id),                       \
	}
		Z_DEVICE_INIT(name, init_fn, pm, data, config, api, state, deps)

/**
 * @brief Define a @ref device and all other required objects.
@@ -976,10 +933,9 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
	IF_ENABLED(CONFIG_DEVICE_DEPS,                                         \
		   (Z_DEVICE_DEPS_DEFINE(node_id, dev_id, __VA_ARGS__);))      \
                                                                               \
	Z_DEVICE_BASE_DEFINE(node_id, dev_id, name, pm, data, config, level,   \
			     prio, api, state, Z_DEVICE_DEPS_NAME(dev_id));    \
                                                                               \
	Z_DEVICE_INIT_ENTRY_DEFINE(node_id, dev_id, init_fn, level, prio)
	Z_DEVICE_BASE_DEFINE(node_id, dev_id, name, init_fn, pm, data, config, \
			     level, prio, api, state,                          \
			     Z_DEVICE_DEPS_NAME(dev_id))

#if defined(CONFIG_HAS_DTS) || defined(__DOXYGEN__)
/**
+24 −1
Changes for include/zephyr/linker/common-rom/common-rom-kernel-devices.ld: 24 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -20,7 +20,30 @@
		__init_end = .;
	} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)

	ITERABLE_SECTION_ROM_NUMERIC(device, 4)
	/* HACK: needs a new iterable "sub-section" API */
	SECTION_PROLOGUE(device_area,,)
	{
		_device_list_start = .;
		_device_list_EARLY_start = .;
		KEEP(*(SORT(._device.static.EARLY_?_*)));
		KEEP(*(SORT(._device.static.EARLY_??_*)));
		_device_list_PRE_KERNEL_1_start = .;
		KEEP(*(SORT(._device.static.PRE_KERNEL_1_?_*)));
		KEEP(*(SORT(._device.static.PRE_KERNEL_1_??_*)));
		_device_list_PRE_KERNEL_2_start = .;
		KEEP(*(SORT(._device.static.PRE_KERNEL_2_?_*)));
		KEEP(*(SORT(._device.static.PRE_KERNEL_2_??_*)));
		_device_list_POST_KERNEL_start = .;
		KEEP(*(SORT(._device.static.POST_KERNEL_?_*)));
		KEEP(*(SORT(._device.static.POST_KERNEL_??_*)));
		_device_list_APPLICATION_start = .;
		KEEP(*(SORT(._device.static.APPLICATION_?_*)));
		KEEP(*(SORT(._device.static.APPLICATION_??_*)));
		_device_list_SMP_start = .;
		KEEP(*(SORT(._device.static.SMP_?_*)));
		KEEP(*(SORT(._device.static.SMP_??_*)));
		_device_list_end = .;
	} GROUP_ROM_LINK_IN(RAMABLE_REGION, ROMABLE_REGION)

#if defined(CONFIG_GEN_SW_ISR_TABLE) && !defined(CONFIG_DYNAMIC_INTERRUPTS)
	SECTION_PROLOGUE(sw_isr_table,,)
+38 −17
Changes for kernel/init.c: 38 added lines, 17 removed lines.
Original line number Diff line number Diff line
@@ -69,6 +69,14 @@ extern const struct init_entry __init_POST_KERNEL_start[];
extern const struct init_entry __init_APPLICATION_start[];
extern const struct init_entry __init_end[];

extern const struct device _device_list_EARLY_start[];
extern const struct device _device_list_PRE_KERNEL_1_start[];
extern const struct device _device_list_PRE_KERNEL_2_start[];
extern const struct device _device_list_POST_KERNEL_start[];
extern const struct device _device_list_APPLICATION_start[];
/* FIXME: iterable sections used in device.h do not use const? */
extern struct device _device_list_end[];

enum init_level {
	INIT_LEVEL_EARLY = 0,
	INIT_LEVEL_PRE_KERNEL_1,
@@ -82,6 +90,8 @@ enum init_level {

#ifdef CONFIG_SMP
extern const struct init_entry __init_SMP_start[];

extern const struct device _device_list_SMP_start[];
#endif

/*
@@ -230,18 +240,32 @@ __pinned_bss
bool z_sys_post_kernel;

/**
 * @brief Execute all the init entry initialization functions at a given level
 * @brief Execute all devices and init entry initialization functions at a given
 * level
 *
 * @details Invokes the initialization routine for each init entry object
 * created by the INIT_ENTRY_DEFINE() macro using the specified level.
 * The linker script places the init entry objects in memory in the order
 * they need to be invoked, with symbols indicating where one level leaves
 * off and the next one begins.
 * @details Invokes the initialization routine for each device and init entry
 * object created by the SYS_INIT() macro using the specified level. Devices are
 * initialized first, then init entry objects. The linker script places the init
 * entry objects in memory in the order they need to be invoked, with symbols
 * indicating where one level leaves off and the next one begins.
 *
 * @param level init level to run.
 */
static void z_sys_init_run_level(enum init_level level)
{
	static const struct device *dev_levels[] = {
		_device_list_EARLY_start,
		_device_list_PRE_KERNEL_1_start,
		_device_list_PRE_KERNEL_2_start,
		_device_list_POST_KERNEL_start,
		_device_list_APPLICATION_start,
#ifdef CONFIG_SMP
		_device_list_SMP_start,
#endif
		/* End marker */
		_device_list_end,
	};

	static const struct init_entry *levels[] = {
		__init_EARLY_start,
		__init_PRE_KERNEL_1_start,
@@ -254,18 +278,14 @@ static void z_sys_init_run_level(enum init_level level)
		/* End marker */
		__init_end,
	};
	const struct init_entry *entry;

	for (entry = levels[level]; entry < levels[level+1]; entry++) {
		const struct device *dev = entry->dev;

		if (dev != NULL) {
	for (const struct device *dev = dev_levels[level]; dev < dev_levels[level+1]; dev++) {
		int rc = 0;

			if (entry->init_fn.dev != NULL) {
				rc = entry->init_fn.dev(dev);
				/* Mark device initialized. If initialization
				 * failed, record the error condition.
		if (dev->init != NULL) {
			rc = dev->init(dev);
			/* Mark device initialized. If initialization failed,
			 * record the error condition.
			 */
			if (rc != 0) {
				if (rc < 0) {
@@ -284,9 +304,10 @@ static void z_sys_init_run_level(enum init_level level)
			/* Run automatic device runtime enablement */
			(void)pm_device_runtime_auto_enable(dev);
		}
		} else {
			(void)entry->init_fn.sys();
	}

	for (const struct init_entry *entry = levels[level]; entry < levels[level+1]; entry++) {
		(void)entry->init_fn.sys();
	}
}

+0 −37
Changes for tests/lib/devicetree/devices/src/main.c: 0 added lines, 37 removed lines.
Original line number Diff line number Diff line
@@ -62,43 +62,6 @@ DEVICE_DT_DEFINE(TEST_NOLABEL, dev_init, NULL,
#define DEV_HDL(node_id) device_handle_get(DEVICE_DT_GET(node_id))
#define DEV_HDL_NAME(name) device_handle_get(DEVICE_GET(name))

ZTEST(devicetree_devices, test_init_get)
{
	/* Check device pointers */
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIO)->dev,
		      DEVICE_DT_GET(TEST_GPIO), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_I2C)->dev,
		      DEVICE_DT_GET(TEST_I2C), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVA)->dev,
		      DEVICE_DT_GET(TEST_DEVA), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVB)->dev,
		      DEVICE_DT_GET(TEST_DEVB), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIOX)->dev,
		      DEVICE_DT_GET(TEST_GPIOX), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVC)->dev,
		      DEVICE_DT_GET(TEST_DEVC), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_PARTITION)->dev,
		      DEVICE_DT_GET(TEST_PARTITION), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIO_INJECTED)->dev,
		      DEVICE_DT_GET(TEST_GPIO_INJECTED), NULL);
	zassert_equal(DEVICE_INIT_GET(manual_dev)->dev,
		      DEVICE_GET(manual_dev), NULL);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_NOLABEL)->dev,
		      DEVICE_DT_GET(TEST_NOLABEL), NULL);

	/* Check init functions */
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIO)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_I2C)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVA)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVB)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIOX)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_DEVC)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_PARTITION)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_GPIO_INJECTED)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_GET(manual_dev)->init_fn.dev, dev_init);
	zassert_equal(DEVICE_INIT_DT_GET(TEST_NOLABEL)->init_fn.dev, dev_init);
}

ZTEST(devicetree_devices, test_init_order)
{
	zassert_equal(init_order[0], DEV_HDL(TEST_GPIO));