[RFC][WIP] Make DEVICE init and device_get dt aware

[galak]

This is prototype based on conversations at ELC-E 2019 on how we can make the device api be dt aware.

The idea here is that we'd use a unique identifier (Zephyr Device ID/ZDID) for each device tree node that would correspond to how we name the struct device for that device. We can then generate various DT defines like DT_INST_0_SILABS_GECKO_I2C_ZDID. These defines allow us to construct direct access to the struct device. This PR show changes to the silabs i2c gecko driver and a hacked up change to the i2c_scanner to show but the changes on the driver side (how a DEVICE_AND_API_INIT_DT would look like, and how device_get_dt_binding would work.

There's at least one issue that needs to be figured out on how / where to do an extern struct device __device_<ZDID>.

[galak]

Benefits of this approach:

• ability to remove the 'label' property from DT nodes [ been a long term goal]
• ability to remove the device name and thus string storage
• code and perf improvement for device_get_dt_* -- its just a pointer dereference now!

[zephyrbot]

Some checks failed. Please fix and resubmit.

pylint issues

************* Module gen_defines
scripts/dts/gen_defines.py:712:0: W0311: Bad indentation. Found 7 spaces, expected 8 (bad-indentation)

checkpatch (informational only, not a failure)

-:10: WARNING:FUNCTION_ARGUMENTS: function definition argument 'counter_gecko_0' should also have an identifier name
#10: FILE: drivers/counter/counter_gecko_rtcc.c:323:
+struct device DEVICE_NAME_GET(counter_gecko_0);

-:23: WARNING:FUNCTION_ARGUMENTS: function definition argument 'dw_dma0' should also have an identifier name
#23: FILE: drivers/dma/dma_cavs.c:365:
+struct device DEVICE_NAME_GET(dw_dma0);

-:36: WARNING:FUNCTION_ARGUMENTS: function definition argument 'dma0_nios2' should also have an identifier name
#36: FILE: drivers/dma/dma_nios2_msgdma.c:207:
+struct device DEVICE_NAME_GET(dma0_nios2);

-:49: WARNING:FUNCTION_ARGUMENTS: function definition argument 'dma0_sam' should also have an identifier name
#49: FILE: drivers/dma/dma_sam_xdmac.c:349:
+struct device DEVICE_NAME_GET(dma0_sam);

-:75: WARNING:FUNCTION_ARGUMENTS: function definition argument 'eth0_sam_gmac' should also have an identifier name
#75: FILE: drivers/ethernet/eth_sam_gmac.c:1933:
+struct device DEVICE_NAME_GET(eth0_sam_gmac);

-:88: WARNING:FUNCTION_ARGUMENTS: function definition argument 'eth_smsc911x_0' should also have an identifier name
#88: FILE: drivers/ethernet/eth_smsc911x.c:658:
+struct device DEVICE_NAME_GET(eth_smsc911x_0);

-:114: WARNING:FUNCTION_ARGUMENTS: function definition argument 'eth0_stm32_hal' should also have an identifier name
#114: FILE: drivers/ethernet/eth_stm32_hal.c:492:
+struct device DEVICE_NAME_GET(eth0_stm32_hal);

-:127: WARNING:FUNCTION_ARGUMENTS: function definition argument 'gpio_cc32xx_a0' should also have an identifier name
#127: FILE: drivers/gpio/gpio_cc32xx.c:205:
+struct device DEVICE_NAME_GET(gpio_cc32xx_a0);

-:136: WARNING:FUNCTION_ARGUMENTS: function definition argument 'gpio_cc32xx_a1' should also have an identifier name
#136: FILE: drivers/gpio/gpio_cc32xx.c:235:
+struct device DEVICE_NAME_GET(gpio_cc32xx_a1);

-:145: WARNING:FUNCTION_ARGUMENTS: function definition argument 'gpio_cc32xx_a2' should also have an identifier name
#145: FILE: drivers/gpio/gpio_cc32xx.c:265:
+struct device DEVICE_NAME_GET(gpio_cc32xx_a2);

-:154: WARNING:FUNCTION_ARGUMENTS: function definition argument 'gpio_cc32xx_a3' should also have an identifier name
#154: FILE: drivers/gpio/gpio_cc32xx.c:295:
+struct device DEVICE_NAME_GET(gpio_cc32xx_a3);

-:291: WARNING:FUNCTION_ARGUMENTS: function definition argument 'i2s0_sam' should also have an identifier name
#291: FILE: drivers/i2s/i2s_sam_ssc.c:954:
+struct device DEVICE_NAME_GET(i2s0_sam);

-:304: WARNING:FUNCTION_ARGUMENTS: function definition argument 'gna' should also have an identifier name
#304: FILE: drivers/neural_net/intel_gna.c:45:
+struct device DEVICE_NAME_GET(gna);

-:317: WARNING:FUNCTION_ARGUMENTS: function definition argument 'uart_cc32xx_0' should also have an identifier name
#317: FILE: drivers/serial/uart_cc32xx.c:33:
+struct device DEVICE_NAME_GET(uart_cc32xx_0);

-:330: WARNING:FUNCTION_ARGUMENTS: function definition argument 'uart_msp432p4xx_0' should also have an identifier name
#330: FILE: drivers/serial/uart_msp432p4xx.c:34:
+struct device DEVICE_NAME_GET(uart_msp432p4xx_0);

-:343: WARNING:FUNCTION_ARGUMENTS: function definition argument 'wdt_sam' should also have an identifier name
#343: FILE: drivers/watchdog/wdt_sam.c:35:
+struct device DEVICE_NAME_GET(wdt_sam);

-:356: WARNING:FUNCTION_ARGUMENTS: function definition argument 'wdt_sam0' should also have an identifier name
#356: FILE: drivers/watchdog/wdt_sam0.c:22:
+struct device DEVICE_NAME_GET(wdt_sam0);

- total: 0 errors, 17 warnings, 407 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[erwango]

ability to remove the 'label' property from DT nodes [ been a long term goal]

I'm still not convinced this is something desirable.
Or at least I think information held in 'label' is a valid an pertinent information.
Referencing to instance, ie index of the enabled peripheral is actually random from board configuration point of view, and so referencing to instance from application perspective is misleading. Besides, it means nothing neither from SoC documents point of view, (ref mans and so on).
If label is removed, we need to be able to get access from the code to device node name, which hold same information.

[pabigot]

Too bad I'm not there; this is exactly the sort of thing I'm interested in.

In principle I think this is great. I'd intended to use the ordinal generated by #19454 for this, with the thought of providing a lookop table rather than external name, but this works. A benefit of using the ordinal is that we could put all the device instance records into a linker section in dependency order, which could simplify initialization.

One concern is that the generated names like DT_INST_0_NORDIC_NRF_SAADC_ZDID intrude on the binding namespace, since it conflicts with a binding property named ZDID. We've made progress to eliminating that by using prefixes instead; it's sad to introduce a new case.

To @erwango's point I think keeping the label property is fine: it should just be used for its defined purpose "human readable string describing a device" rather than as a handle that identifies the device within a registry. Though I don't know how we deal with all the device_get_binding(DT_INST_0_FOO_LABEL) uses that already exist.

[pabigot]

It would be nice if this include some identification:

#ifndef _ASMLANGUAGE
extern struct device __device_4;   /* SILABS_GETCKO_I2C#0 */
#endif

or something else so we could catch the declaration name using grep. Given the amount of commenting in the generated file this looks bare without the identification.

[galak]

Will need @ulfalizer help with that, but should be doable.

[pabigot]

Here's a case where @erwango's point becomes highly relevant. When I see code like this I usually change it to:

printk("I2C: Device driver %s not found.\n", I2C_DEV);

(and have done so in this exact code multiple times, though never pushed it up). A human can read "Device driver I2C_0 not found" and realize that I2C_1 should have been specified instead.

Can we add API to do something like:

printk("I2C: Device driver %s not found.\n", device_get_dt_label(SILABS_GECKO_I2C, 0));

[pabigot]

It would be nice if this include some identification:

...
extern struct device __device_4;   /* SILABS_GETCKO_I2C#0 */

or something else so we could catch the declaration name using grep. I'm imagining cases where I have a pointer, look it up in a linker map, and want to know what it belongs to.

[ulfalizer]

Some nits. Need to look into how this stuff works atm in more detail.

[ulfalizer]

Could move into a separate function (init_zdid() or init_dev_ids() or the like). Would be nice if we could keep main() about the same size forever.

[galak]

I'm still not convinced this is something desirable.
Or at least I think information held in 'label' is a valid an pertinent information.
Referencing to instance, ie index of the enabled peripheral is actually random from board configuration point of view, and so referencing to instance from application perspective is misleading. Besides, it means nothing neither from SoC documents point of view, (ref mans and so on).
If label is removed, we need to be able to get access from the code to device node name, which hold same information.

The application can reference via instance, alias, path. I just did it via instance in here.

so we can have:

device_get_dt_by_path(SOC_I2C_4000C400) [maps to /soc/i2c@4000C400]
device_get_dt_by_alias(I2C_0)
etc.

[ulfalizer]

The application can reference via instance, alias, path. I just did it via instance in here.

so we can have:

device_get_dt_by_path(SOC_I2C_4000C400) [maps to /soc/i2c@4000C400]
device_get_dt_by_alias(I2C_0)
etc.

Seems pretty neat.

Guess the aliases generated by out_dev() are needed in that case. Would expand to a different *_ZDID alias depending on how you look it up, and then that gets resolved to the right device struct.

Still wonder if there's a nicer name than ZDID. Could make it *__ZDID (or just *__ID) maybe, corresponding to _id = ..., though it might be overkill. Nice to mark it "magic" somehow at least.

[erwango]

The application can reference via instance, alias, path. I just did it via instance in here:

device_get_dt_by_path(SOC_I2C_4000C400) [maps to /soc/i2c@4000C400]

I don't think this scales, peripheral address is not used in ref manuals (it is not even available) nor available on most of board documentation. User have to go and check:

• CMSIS file (in which it is not even directly available #define GPIOH_BASE (AHB1PERIPH_BASE + 0x1C00UL)).
• .dtsi (which are split in multiple files, so need to parse several files)

device_get_dt_by_alias(I2C_0)

So we need to define an alias for each peripheral instance... Not sure this scales either.

Anyway we can use node name (ie phandle)?

[mbolivar]

Anyway we can use node name (ie phandle)?

The vision for this that I discussed with Kumar was that "it's just a pointer dereference!" makes phandles "just work": from prop = <&gpioM ...> you can translate the &gpioM phandle to &__device_N in the generated header "right away", since the script knows the ID number for the node pointed to by the phandle.

Edit: to clarify, M != N. gpioM is just an arbitrary DT label. The number N is the zdid for the device struct corresponding to the node so labeled.

[pabigot]

Anyway we can use node name (ie phandle)?

The node name isn't the phandle, which isn't text; the generic syntax is:

    [label:] node-name[@unit-address] { 
      [phandle = <*int32*>;]
      ... 
   }

The label is what's used as the reference in things like <&gpiof 1 0>, and it evaluates (in blob representations) to the integer that is the (usually implicit) phandle property. The node-name need only be unique within its parent, so it's not a good choice.

Nomenclature nit-picking aside, I agree that device_get_dt_by_label() would be useful, if we can figure out how to name it so it's not confused with the label property.

[pabigot]

The vision for this that I discussed with Kumar was that "it's just a pointer dereference!" makes phandles "just work": from prop = <&gpioM ...> you can translate the &gpioM phandle to &__device_N in the generated header "right away", since the script knows the ID number for the node pointed to by the phandle.

This makes sense.

Something to consider is that AFAIK to this point the only way to get a struct device * was to use device_get_binding(LABEL), and the result is null if the device exists but does not function.

device_get_dt_binding*() appears to return a pointer to a struct device whether or not that device works (and will fail to link if the referenced device was not instantiated). This is a "good thing" as it enables progress on #19448 where we need to be able to turn off devices, or turn on devices that weren't enabled at boot. However, we do need to take care that driver access is made safe against invocations where the pointer was obtained without validating the device or before the device was started. That may require a tree-wide inspection, or something injected at the driver wrapper level.

[galak]

Too bad I'm not there; this is exactly the sort of thing I'm interested in.

In principle I think this is great. I'd intended to use the ordinal generated by #19454 for this, with the thought of providing a lookop table rather than external name, but this works. A benefit of using the ordinal is that we could put all the device instance records into a linker section in dependency order, which could simplify initialization.

Agreed, the intent was the assign the numbers in that order, I was just being quick with giving some numbering system to show something work. But totally agree this should be

One concern is that the generated names like DT_INST_0_NORDIC_NRF_SAADC_ZDID intrude on the binding namespace, since it conflicts with a binding property named ZDID. We've made progress to eliminating that by using prefixes instead; it's sad to introduce a new case.

Fair, I thought about that and agree. This will get fixed.

To @erwango's point I think keeping the label property is fine: it should just be used for its defined purpose "human readable string describing a device" rather than as a handle that identifies the device within a registry. Though I don't know how we deal with all the device_get_binding(DT_INST_0_FOO_LABEL) uses that already exist.

The hope is to remove the string at some point since it would reduce footprint.

[pabigot]

To @erwango's point I think keeping the label property is fine: it should just be used for its defined purpose "human readable string describing a device" rather than as a handle that identifies the device within a registry. Though I don't know how we deal with all the device_get_binding(DT_INST_0_FOO_LABEL) uses that already exist.

The hope is to remove the string at some point since it would reduce footprint.

I was thinking of a Kconfig option that would cause the value of the label property to be stored in the device device_config structure. This would allow access to that property from user code, and would contribute to solving the problem of displaying device information in a human-readable way at runtime.

[pabigot]

... use the ordinal generated by #19454 ...
Agreed, the intent was the assign the numbers in that order

That would allow us to use the ordinal (ZDID) as the opaque device handle, with -1 to represent an unknown device (or 0 if we muck with the device array indexing, so references initialize to "no known device").

Then there's still a barrier of going through device_get_binding_by_id() to get a validated struct device *.

[tbursztyka]

I am not sure we want to separate the base identifier of a device and its instance number.
First because it will require to deprecate device_get_binding(), second because its replacement will require 2 parameters (ok that's a minor issue, but it adds up a bit of complexity: before 1 param, now 2... to do the same thing).

[tbursztyka]

Why not instead including it as the other generated_dts_*.h files?

[galak]

Not sure I follow.

[tbursztyka]

including it in generated_dts_board.h
I find it awkward to have this new generated header in a public header file such as device.h

[galak]

There's an issue in that generated_dts_board.h is just defines, putting externs in there causes issues with the linker scripts as they include and I think utilize defines from generated_dts_board.h

[mbolivar]

can't we #ifndef _LINKER_SCRIPT our way around that?

[pabigot]

Or advance to putting them all into a linker section in ZDID order, and indexing into the resulting array with ZDID. A benefit of that is that the opaque handle could be a u16_t, we could retain device_get_binding(ID) with a change in argument signature (or a parallel function with different name), and we don't have to worry as much about people trying to use uninitialized devices just because they got the pointer to one somehow. Then there's just:

extern struct device *__devices[];
#define device_get_dt_binding(BLAH) __devices[MAGIC_GET_ZDID(BLAH)]

somewhere.

(I assume one ordinal reserved for "no-such-device", and no Zephyr image would have more than 65534 devices. 254 seems too small.)

[galak]

can't we #ifndef _LINKER_SCRIPT our way around that?

possibly, didn't know this ifdef existed.

[mbolivar]

possibly, didn't know this ifdef existed.

The name is just a suggestion, I'm saying we can make sure a macro is defined only when running cpp over the ld scripts and use that here

[tbursztyka]

hum, if there was an issue with you approach that would be this: the need to replace device_get_binding() by something like above.

Can't we find a way to avoid that? (I fully realize if there is a solution, it will require all your patches to be reworked.)

[galak]

avoid it how?

[galak]

first, what would be better?

[tbursztyka]

avoid it how?

Exactly my question :)

I understand the idea behind this patches, however it's changing how user will get the bindings. It could be done directly with a static string being the same as the dts node label ("SPI_0" for instance). That was simple.

Also doing it that way was closely down to the board the user is using. He probably does not know which controller runs on the SoC, he has a board that exposes "spi_0, spi_1, uart_0, uart_1, uart_2" and it is then direct: device_get_binding("UART_0"). One can relate it directly to the board.

Now it would require the user to know the controller, and the instance he needs, exposing then the instance concept to the user at the same time then... not sure it's wise.

Looks like I am in line with @erwango here.

[mbolivar]

Keeping the labels available under a default y kconfig would let users keep doing this for upstream samples but still let us get rid of the labels to save ROM when needed, agreed @tbursztyka ?

[tbursztyka]

When it comes to name, I think we want to get rid of them entirely. They are really useless.
(only maybe in debug mode, inside the actual driver, but nowhere else).

Instead we could generate BOARD_UART_0, BOARD_SPI_1, ... macros which in fact would hold the dev name (which is being build with the ZDID etc.. on this rfc). device_get_binding() would then call DEVICE_GET() with this, and since dts_device_extern.h already exposes the extern device pointers: done.

@galak That could be your solution to avoid fully deprecating device_get_binding(), though it would change it's signature (it would become a macro calling DEVICE_GET() macro).

[galak]

I expect several device_get APIs that would be new. We can continue to support device_get_binding() via string. I'd expect we'd have:

device_get_by_dt_label(LABEL) [this is the node label, not the 'label' property]
device_get_by_dt_alias(ALIAS)
device_get_by_dt_path(PATH)
device_get_by_dt_inst(NAME, ID) [not sure if this one is useful]

[tbursztyka]

ok sure, device_get_binding() would use device_get_by_dt_label then.

[jfischer-no]

device_get_by_dt_inst(NAME, ID) [not sure if this one is useful]

If the NAME is type of the device and ID the index, then it would allow the same feature as suggested in #21709