Skip to content
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

scripts: dts: Use DT_ prefix in generated labels #11180

Merged
merged 5 commits into from
Nov 13, 2018

Conversation

anangl
Copy link
Member

@anangl anangl commented Nov 7, 2018

As agreed at ELCE, these are the changes required to proceed further with #10377.

I wasn't sure how to treat some FLASH related labels (like FLASH_WRITE_BLOCK_SIZE) since they are directly referenced from existing code. I decided to leave them in the current form, see the commit message.

Almost all labels generated by the extracting script are now prefixed
with DT_. The only exceptions are:
- stuff with 'base_label' specified in yaml bindings
- items specified by 'regs_config' and 'name_config' dictionaries
  in globals.py module
- FLASH related labels generated by flash.extract() called separately
  from generate_node_definitions(), e.g. FLASH_WRITE_BLOCK_SIZE -
  these are used directly, not through fixups, from existing code
  so I didn't want to touch them now

Labels generated for aliases are additionally prefixed with information
from the 'compatible' property, e.g. DT_GPIO_LEDS_LED0_* is generated
instead of LED0_*. To provide backward compatibility for code that uses
LEDx_* and SWx_* labels in their previous forms, a command line option
named 'old-alias-names' is added to the extraction script. This option
causes that the labels for aliases are generated in both old and new
forms. Currently this option is always enabled in dts.cmake.

Signed-off-by: Andrzej Głąbek <[email protected]>
All labels containing "_<8-hex-digits>_" or "16550_<3or6-hex-digits>_"
in their names, assumed to be generated by the extracting script,
are updated with the DT_ prefix, to reflect the recent changes made
to the script.

Signed-off-by: Andrzej Głąbek <[email protected]>
Update a couple of labels generated from DTS used directly (not through
dts_fixups) in some serial drivers, to the reflect recent changes made
to the extracting script.

Signed-off-by: Andrzej Głąbek <[email protected]>
Update a couple of labels generated from DTS used directly (not through
dts_fixups) in TI CC2650 system initialization code and a few drivers
for this SoC.

Signed-off-by: Andrzej Głąbek <[email protected]>
These changes were obtained by running a script  created by
Ulf Magnusson <[email protected]> for the following
specification:

1. Read the contents of all dts_fixup.h files in Zephyr
2. Check the left-hand side of the #define macros (i.e. the X in
   #define X Y)
3. Check if that name is also the name of a Kconfig option
   3.a If it is, then do nothing
   3.b If it is not, then replace CONFIG_ with DT_ or add DT_ if it
       has neither of these two prefixes
4. Replace the use of the changed #define in the code itself
   (.c, .h, .ld)

Additionally, some tweaks had to be added to this script to catch some
of the macros used in the code in a parameterized form, e.g.:
- CONFIG_GPIO_STM32_GPIO##__SUFFIX##_BASE_ADDRESS
- CONFIG_UART_##idx##_TX_PIN
- I2C_SBCON_##_num##_BASE_ADDR
and to prevent adding DT_ prefix to the following symbols:
- FLASH_START
- FLASH_SIZE
- SRAM_START
- SRAM_SIZE
- _ROM_ADDR
- _ROM_SIZE
- _RAM_ADDR
- _RAM_SIZE
which are surprisingly also defined in some dts_fixup.h files.

Finally, some manual corrections had to be done as well:
- name##_IRQ -> DT_##name##_IRQ in uart_stm32.c

Signed-off-by: Andrzej Głąbek <[email protected]>
@galak galak merged commit 2020290 into zephyrproject-rtos:master Nov 13, 2018
@galak
Copy link
Collaborator

galak commented Nov 13, 2018

Going to merge it, and we'll deal with any fixup, etc after.

@anangl anangl deleted the dt_prefixes branch November 13, 2018 17:04
nvlsianpu added a commit to nvlsianpu/mcuboot that referenced this pull request Nov 14, 2018
zephyrproject-rtos/zephyr#11180: Zephyr target was corrupted as
recently zephyr's device tree started adding DT_ prefix in
generated labels.

This path aligns flash name macro used.


Signed-off-by: Andrzej Puzdrowski <[email protected]>
carlescufi pushed a commit to mcu-tools/mcuboot that referenced this pull request Nov 14, 2018
zephyrproject-rtos/zephyr#11180: Zephyr target was corrupted as
recently zephyr's device tree started adding DT_ prefix in
generated labels.

This path aligns flash name macro used.


Signed-off-by: Andrzej Puzdrowski <[email protected]>
@mnkp
Copy link
Member

mnkp commented Nov 28, 2018

This PR has been merged however I would like to add one comment.

What is the rational for adding compat in alias prefix? Won't we loose genericity?

To indicate the purpose of the value that the label defines and this way help preventing misuses of the label (see #10377, we want to use labels generated through aliases in drivers, to avoid creating fixups). [...]
We won't loose genericity. For generic nodes we'll still get generic prefixes, for instance DT_GPIO_LEDS_ for "gpio-leds" compatibles. The only disadvantage I see is that the label name lengthens, but I guess it is an acceptable price.
-- @anangl

According to the Devicetree Specification "The alias node shall be at the root of the devicetree". Which implies that properties defined by the node belong to the global namespace. We probably shouldn't prefix defines generated from the aliases node with 'compatible' property.

@anangl
Copy link
Member Author

anangl commented Nov 29, 2018

According to the Devicetree Specification "The alias node shall be at the root of the devicetree". Which implies that properties defined by the node belong to the global namespace. We probably shouldn't prefix defines generated from the aliases node with 'compatible' property.

But please note that we don't generate labels for the properties of the "aliases" node themselves. These properties (i.e. actual definitions of aliases) provide only alternative (usually shorter) paths to other nodes defined in the Devicetree, and they are used here exactly for this purpose. Therefore, for every node for which an alias is also defined, we generate an extra set of labels with the middle part (normally derived from the path to the node) replaced with the alias name.

If I am not precise enough in my explanation, I hope @galak can correct me or elaborate on this. In one of the comments above he already stated:

We should have the alias replace a very specific part of symbol name we create.

@pabigot
Copy link
Collaborator

pabigot commented Nov 29, 2018

@anangl If add_prop_aliases in extract/globals.py is modified to prefix DT_ to what's emitted under --old-alias-names I think we'd get something pretty close to what is being requested. Though there might be some multiple-definition issues to catch.

@mnkp
Copy link
Member

mnkp commented Nov 29, 2018

Therefore, for every node for which an alias is also defined, we generate an extra set of labels with the middle part (normally derived from the path to the node) replaced with the alias name.

From the Devicetree Specification: "Each property of the /aliases node defines an alias. The property name specifies the alias name. The property value specifies the full path to a node in the devicetree". It implies that an alias can replace the full path to the node name.

If we remove 'compatible' from the defines generated for aliases it doesn't stop us from using them in the device drivers and getting rid of the dts_fixup.h files. Every alias refers to a single, distinct hardware module.

Also, the current approach of prefixing defines generated for aliases with 'compatible' property doesn't solve any problem, instead it creates some:

So indeed, I was wondering on non generic nodes, like for instance, if we have:

	aliases {
		zi2c = &i2c1;
	};

So generated #define will use i2c1 compatible field.
Which means we won't be able to use a ZI2C_NAME for testing for instance. And we'll always need fixup to establish generic #define's like
#define CONFIG_I2C_1_NAME ST_STM32_I2C_V2_40005400_LABEL
Or we'll need other approach than alias to establish generic devices bindings
-- @erwango

Yes. It won't be possible to use aliases for this.
how can we solve this? Using chosen maybe?
-- @anangl

@mnkp
Copy link
Member

mnkp commented Nov 29, 2018

Just one more comment. If, for some reason, we would prefer to use a 'qualified' names in the defines there is a different approach which does not involve misusing aliases node.

According to the specification a DTS node is defined as

[label:] node-name[@unit-address] {
	[properties definitions]
	[child nodes]
}

e.g.

usart0: usart@40010000 { /* USART0 */
		[...]
};

where usart0 is a node label. We could modify the script to generate DT_ defines that use label name rather than a unit address. E.g.

DT_SILABS_GECKO_USART_40010000_BASE_ADDRESS=0x40010000

would become

DT_SILABS_GECKO_USART_USART0_BASE_ADDRESS=0x40010000

As such the resulting defines would rely on logical names and not on part number dependent, fixed addresses. Such defines could be used directly by the device drivers without the need for dts_fixup entries.

@pfalcon
Copy link
Contributor

pfalcon commented Nov 29, 2018

would become
DT_SILABS_GECKO_USART_USART0_BASE_ADDRESS=0x40010000

@mnkp: You're touching on very important issue, which drills back of my mind for a long time. Suppose I wrote driver_foo.c, how to make it work for board1 with foo@10000000 and board2 with foo@20000000?

So, please continue! I'm happy to give +0.9 for any suitable proposal which addresses that sanely (+0.9 because I don't think U know DT enough to be 100% sure that some solution is the best one).

@mnkp
Copy link
Member

mnkp commented Dec 4, 2018

Let's continue discussion related to use of aliases, node labels and other issues which goal is to eliminate dts_fixup files to #11737 since this PR has already been merged and lists many participants who are likely not interested in the topic.

pabigot added a commit to pabigot/zephyr that referenced this pull request Dec 9, 2018
The rationale for encoding compatible identifier in the alias prefix was
in support of a feature (Kconfig-based identification of compatible
drivers) that has not yet been designed/implemented.

Since it's causing trouble now, add a more generic alias.  Leave the
compat-annotated one in place with the expectation of removing it before
1.14.

See: zephyrproject-rtos#10821 (comment)
See: zephyrproject-rtos#11180 (comment)
See: zephyrproject-rtos#11180 (comment)
See: zephyrproject-rtos#11737
Signed-off-by: Peter A. Bigot <[email protected]>
filip-proglove pushed a commit to filip-proglove/mcuboot that referenced this pull request Aug 16, 2019
zephyrproject-rtos/zephyr#11180: Zephyr target was corrupted as
recently zephyr's device tree started adding DT_ prefix in
generated labels.

This path aligns flash name macro used.


Signed-off-by: Andrzej Puzdrowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.