Skip to content

Conversation

@mnkp
Copy link
Member

@mnkp mnkp commented Nov 28, 2018

Following PR is a request for comments. The change is implemented for efr32fg1p SoC only.

By adding 'aliases' node in SoC .dtsi file it is possible to generate DT_ defines which specify a logical name rather than relay on a module location on APB bus. E.g. DT_SILABS_GECKO_USART_40010000_LABEL becomes DT_SILABS_GECKO_USART_USART_0_LABEL. Thus it is possible to remove dts_fixup.h defines. There is no limit on adding aliases for a given node and user is free to add a different one. Nordic family SoCs use the same approach.

Another possibility to avoid adding dts_fixup entries is to modify the script which generates DT_ defines. 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. If a script used a node label rather than a unit address the resulting defines would relay on logical names and not on part number dependent, fixed addresses. However, since this approach was not implemented by maintainers of DTS scripts there may be a good reason to avoid it.

@mnkp mnkp added DNM This PR should not be merged (Do Not Merge) platform: Silabs Silicon Labs labels Nov 28, 2018
@mnkp mnkp requested a review from chrta November 28, 2018 14:00
@mnkp
Copy link
Member Author

mnkp commented Nov 28, 2018

@ekarlso, @diegosueiro, @mtuxpe I've prepared another PR as an RFC with a goal to remove reliance on dts_fixup.h file. The change was done for efr32fg1p series only. If this approach is accepted remaining SiLabs series would also need to be updated. Your comments are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The USART string repetition in the generated #define seems unnecessary to me.
@galak, is there a way to avoid this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern was established by #11180. The first part 'SILABS_GECKO_USART' is equal to the 'compatible' property of the node for which the define is generated. What follows is the extracted property itself.

One could argue that this is not the right choice for the define generated from an alias node since according to the device tree specification "The alias node shall be at the root of the devicetree". That means it belongs to the global namespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnkp,

I definitely agree on using aliases over dts_fixup. It is just strange for me (almost hurt my eyes) seeing this string repetitions.

@codecov-io
Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #11707 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11707   +/-   ##
=======================================
  Coverage    48.2%    48.2%           
=======================================
  Files         280      280           
  Lines       43323    43323           
  Branches    10374    10374           
=======================================
  Hits        20886    20886           
  Misses      18289    18289           
  Partials     4148     4148

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c4bb10...8b9c18b. Read the comment docs.

@mnkp mnkp requested a review from anangl November 28, 2018 15:07
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one question, maybe related to this. I plan to implement an spi driver that uses the usart peripheral.
In the device tree, this could use the same base node, e.g. usart@40010000, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, DTS describes underlying hardware and its structure. Since both drivers use the same hardware module they will use the same dts node.

One example that I know of where the same hardware module is used by several drivers is sercom peripheral on Atmel SAMD21 device. Best place to start looking into the code is here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Copy link
Contributor

@chrta chrta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using aliases over dts_fixup.h

By adding 'aliases' node in SoC .dtsi file it is possible to generate
DT_ defines which specify a logical name rather than relay on module
location on APB bus. E.g. DT_SILABS_GECKO_USART_40010000_LABEL becomes
DT_SILABS_GECKO_USART_USART_0_LABEL. Thus it is possible to remove
dts_fixup.h defines.

Signed-off-by: Piotr Mienkowski <[email protected]>
@mnkp mnkp force-pushed the silabs_exx32_dts_fixup branch from cd9c388 to 8b9c18b Compare November 29, 2018 19:00
@mnkp
Copy link
Member Author

mnkp commented Nov 29, 2018

I've pushed a new version that implements required changes for all SiLabs SoCs and boards. This PR is now suitable for merging.

I confirmed that all SiLabs boards compile with no error. However, some more testing would definitely be welcomed. Especially verifying that i2c, usart, uart, leuart modules work in real life on the board.

@mnkp mnkp removed the DNM This PR should not be merged (Do Not Merge) label Nov 29, 2018
@mnkp mnkp changed the title RFC: dts: silabs: use 'aliases' to remove dts_fixup defines dts: silabs: use 'aliases' to remove dts_fixup defines Nov 29, 2018
@mnkp
Copy link
Member Author

mnkp commented Jan 7, 2019

@galak, @MaureenHelm The discussion regarding the best method to remove dts_fixup files is still ongoing, however the aliases are already used by the Nordic SoCs. Even if we choose to handle generation of DTS defines differently it's easier to update defines within a few device drivers rather than maintain dts_fixup files. Could we review and merge this PR?

@galak galak merged commit 973af2c into zephyrproject-rtos:master Jan 8, 2019
@mnkp mnkp deleted the silabs_exx32_dts_fixup branch January 8, 2019 20:36
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.

5 participants