Skip to content

scripts: kconfig: Add dt_compat_get_inst_prop method #21560

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

Closed

Conversation

nandojve
Copy link
Member

@nandojve nandojve commented Dec 21, 2019

The dt_str_val method has been deprecated. This add an alternative
that uses compat and instance number to find a node and than
return the string representation or a passed default value.

This is necessary becase path is valid only at SoC directory. For
instance, a driver for a SPI bus need search for a compat based on
the instance to determine the proper configuration.

depends on: #22215

Example: how determine the right interface name from the device tree
node label.

at DT:

&foo0 {
	compatible = "manufacturer,device"
	status = "disabled";
	label = "FOO_0";
};
&foo1 {
	compatible = "manufacturer,device"
	status = "okay";
	label = "FOO_1";
};

at Kconfig:

DT_FOO      := manufacturer,device
DT_FOO_INST := 0
DT_FOO_DEF  := "default string on not match/error - optional empty"

config FOO
	string "text"
	default "$(dt_compat_get_str, \
		 label, \
		 $(DT_FOO), \
		 $(DT_FOO_INST), \
		 $(DT_FOO_DEF))"

The result is CONFIG_FOO="FOO_1"

An example of use was delivered too.

@nandojve
Copy link
Member Author

Rebase and Fix 364 line.

        if prop not in node.props:
            continue

to

        if prop not in node.props:
            break

Copy link
Collaborator

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Some nits and random thoughts.

Having some generic property lookup functions like these seems nice.

@ulfalizer ulfalizer requested a review from galak December 23, 2019 08:59
@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from ba17e04 to d0a3c95 Compare December 23, 2019 13:28
@nandojve
Copy link
Member Author

nandojve commented Dec 23, 2019

I assumed that instance_no will be enough to check but testing in deep I saw inconsistencies.

&foo0 {
	status = "disabled";
	bar@0 {
		compatible = "man,bar";
		reg = <0x0>;
		label = "WPAN_0";
		status = "disabled";
	};
	bar@1 {
		compatible = "man,bar";
		reg = <0x1>;
		label = "WPAN_1";
		status = "okay";
	};
};

&foo1 {
	status = "okay";
	bar@0 {
		compatible = "man,bar";
		reg = <0x0>;
		label = "RF2XX_2";
		status = "okay";
	};
}; 

On this example the first instance should be /../foo1/bar@0 and must be the only one. The current version returns /../foo0/bar@0. At generated_dts_board.conf file I saw two instances /../foo0/bar@1 and /../foo1/bar@0.

Added scripts: dts: edtlib.py: Fix DT enabled node property to address the issue and fix unnecessary instances at generated_dts_board.conf

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from d0a3c95 to 1b88247 Compare December 23, 2019 19:02
@nandojve nandojve requested a review from ulfalizer December 23, 2019 19:06
@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 1b88247 to 411eca2 Compare December 24, 2019 18:02
Copy link
Collaborator

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Having status = "disabled" disable all the child nodes as well makes sense to me. Thanks for that.

I wonder how common it is to have nested status properties, but if they show up, it probably makes sense for the parent status to take precedence.

Feels like the old _set_instance_no() code could be cleaned up a bit. It's pretty subtle how it works (depends on edt.nodes only including the "previous" nodes when the function is called).

I'm thinking it might be nice to have an edt.compat2nodes property, where e.g. edt.compat2nodes["foo"] would be a list with all enabled nodes that have compatible = ..., "foo", .... That would simplify dt_compat_get_str() as well.

I'll make a separate PR for that.

@ulfalizer
Copy link
Collaborator

PR that turns Node.instance_no into EDT.compat2nodes: #21592

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 411eca2 to f51ec92 Compare December 27, 2019 00:28
ulfalizer added a commit to ulfalizer/zephyr that referenced this pull request Dec 27, 2019
Add a dictionary EDT.compat2nodes that maps each compatible that appears
on some enabled node to a list of enabled nodes that implement the
compatible. For example, EDT.compat2nodes["foo"] is a list of all
enabled nodes whose 'compatible' includes "foo".

The old Node.instance_no functionality can easily be implemented in
terms of EDT.compat2nodes, so get rid of Node.instance_no.
EDT.compat2nodes is more powerful and easier to understand.

Simplify main() in gen_defines.py a bit by using EDT.compat2nodes to
generate the DT_COMPAT_<compatible> existence macros. The behavior is
slightly different now, as DT_COMPAT_<compatible> is generated for
enabled nodes that don't have a binding as well, but that might be an
improvement overall, and probably doesn't hurt.

EDT.compat2nodes will make the implementation of the new
$(dt_compat_get_str) preprocessor function in
zephyrproject-rtos#21560 cleaner and
simpler. That was the original motivation.

Signed-off-by: Ulf Magnusson <[email protected]>
Copy link
Collaborator

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Seems alright to me. Nice to have a function like that. Could be simplified a bit once #21592 is in too. Suspect most people are vacation at the moment.

Wonder if the function could just be called dt_compat_get too, since it also works for integers. Maybe there could be a dt_compat_get_array later if needed, that also takes an array index.

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from f51ec92 to 0f71741 Compare December 27, 2019 13:30
@nandojve
Copy link
Member Author

Seems alright to me. Nice to have a function like that. Could be simplified a bit once #21592 is in too. Suspect most people are vacation at the moment.

Wonder if the function could just be called dt_compat_get too, since it also works for integers. Maybe there could be a dt_compat_get_array later if needed, that also takes an array index.

I agreed with you, make sense.

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 0f71741 to 9719f22 Compare December 27, 2019 14:19
@nandojve
Copy link
Member Author

Rebase on top of dts: edtlib: Turn Node.instance_no into more flexible EDT.compat2nodes #21592

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 9719f22 to efa06b0 Compare December 28, 2019 16:00
@stephanosio stephanosio closed this Jan 2, 2020
@stephanosio stephanosio reopened this Jan 2, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 2, 2020

All checks passed.

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

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from efa06b0 to 799e1ee Compare January 6, 2020 20:38
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

Mixing up DTS into Kconfig is, imo, a mistake.
After almost 3 years we have been using DTS, we are still not yet where we want to be (too many instance-centric options are still in Kconfig, etc..)
And that PR makes it worse by mixing both. Definitely not a good idea.

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 799e1ee to a7d2561 Compare January 14, 2020 16:20
@nandojve nandojve changed the title scripts: kconfig: Add dt_compat_get_string method scripts: kconfig: Add dt_compat_get_inst_prop method Jan 14, 2020
@nandojve
Copy link
Member Author

Mixing up DTS into Kconfig is, imo, a mistake.
After almost 3 years we have been using DTS, we are still not yet where we want to be (too many instance-centric options are still in Kconfig, etc..)
And that PR makes it worse by mixing both. Definitely not a good idea.

The initial motivation of this PR was an open discussion at #21402. I agreed with you after weeks looking around DT, fixups, Kconfig etc. I believe any driver definition/configuration must be on that place and not around.

I would like mention that there are DT improvements on #21592 and bug fix at 'scripts: dts: edtlib.py: Fix DT enabled node property' that probably need move forward.

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from a7d2561 to 17bc8d2 Compare January 26, 2020 23:28
@nandojve
Copy link
Member Author

Created "scripts: dts: edtlib.py: Fix DT enabled node property" #22215 dependency to separate DT fix from the current PR.

ulfalizer added a commit to ulfalizer/zephyr that referenced this pull request Jan 28, 2020
Add an EDT.compat2enabled attribute that maps compatibles to enabled
devicetree nodes that implement them. For example,
EDT.compat2enabled["foo"] is a list of all enabled nodes with "foo" in
the 'compatible' property.

The old Node.instance_no functionality can be implemented in terms of
EDT.compat2enabled, so remove Node.instance_no. EDT.compat2enabled is
more flexible and easier to understand.

Simplify main() in gen_defines.py by using EDT.compat2enabled to
generate the DT_COMPAT_<compatible> existence macros. The behavior is
slightly different now, as DT_COMPAT_<compatible> is generated for
enabled nodes that don't have a binding as well, but that might be an
improvement overall. It probably doesn't hurt at least.

EDT.compat2enabled simplifies the implementation of the new
$(dt_compat_get_str) preprocessor function in
zephyrproject-rtos#21560. That was the
original motivation.

Signed-off-by: Ulf Magnusson <[email protected]>
galak pushed a commit that referenced this pull request Jan 29, 2020
Add an EDT.compat2enabled attribute that maps compatibles to enabled
devicetree nodes that implement them. For example,
EDT.compat2enabled["foo"] is a list of all enabled nodes with "foo" in
the 'compatible' property.

The old Node.instance_no functionality can be implemented in terms of
EDT.compat2enabled, so remove Node.instance_no. EDT.compat2enabled is
more flexible and easier to understand.

Simplify main() in gen_defines.py by using EDT.compat2enabled to
generate the DT_COMPAT_<compatible> existence macros. The behavior is
slightly different now, as DT_COMPAT_<compatible> is generated for
enabled nodes that don't have a binding as well, but that might be an
improvement overall. It probably doesn't hurt at least.

EDT.compat2enabled simplifies the implementation of the new
$(dt_compat_get_str) preprocessor function in
#21560. That was the
original motivation.

Signed-off-by: Ulf Magnusson <[email protected]>
@galak galak added area: Devicetree Tooling PR modifies or adds a Device Tree tooling and removed area: Devicetree labels Jan 30, 2020
@ulfalizer
Copy link
Collaborator

ulfalizer commented Jan 31, 2020

@tbursztyka

Mixing up DTS into Kconfig is, imo, a mistake.
After almost 3 years we have been using DTS, we are still not yet where we want to be (too many instance-centric options are still in Kconfig, etc..)
And that PR makes it worse by mixing both. Definitely not a good idea.

I think the problem is mostly Kconfig options that allow you to change stuff that's already known from selecting the board.

Think of Kconfig like an options menu in a game. You wouldn't show an option for selecting the operating system there, since it's already known. Kconfig is supposed to be simple like that, but the concepts got lost.

Having devicetree information available to Kconfig is something else though. It allows you to tweak the options based on what the devicetree says (e.g. to only show Windows-specific options if the game runs on Windows, just to extend that example).

This PR might help with getting rid of devicetree.conf too, which would be nice.

@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch 4 times, most recently from a62b4a5 to 1726408 Compare February 1, 2020 22:28
nandojve added 3 commits March 4, 2020 20:24
The current version evaluate the node status and not the parent status.
That define unnecessary symbols at 'devicetree.conf' which can
increases total code size. This fix adding a new initialization method
that evaluate all parents from a node and disable when appropriated.

This fixes 'instance_no' set adding 'compat' only if node is enabled.

Signed-off-by: Gerson Fernando Budke <[email protected]>
The 'dt_str_val' method has been deprecated. This add an alternative
that uses 'compatible' and 'instance index' to find a node and than
return the string representation or a passed default value. The
instance is indexed by occurences of a compatible node with property
'status=okay'.

This is necessary because 'path' is valid only at SoC directory. For
instance, a driver for a SPI bus need search for a 'compatible' based
on the 'instance' to determine the proper configuration.

Example: how determine the right interface name from the device tree
node label for the first instance?

at DT:

&foo0 {
	compatible = "manufacturer,device"
	status = "disabled";
	label = "FOO_0";
};
&foo1 {
	compatible = "manufacturer,device"
	status = "okay";
	label = "FOO_1";
};

at Kconfig:

DT_FOO      := manufacturer,device
DT_FOO_INST := 0
DT_FOO_DEF  := "default string on not match/error - optional empty"

config FOO
	string "text"
	# Fetch 'label' property from instance INST, defaulting to DEF
	default "$(dt_compat_get_inst_prop, \
		 label, \
		 $(DT_FOO), \
		 $(DT_FOO_INST), \
		 $(DT_FOO_DEF))"

The result is CONFIG_FOO="FOO_1".

'dt_compat_get_inst_prop' an be used to fetch properties that store
string or integer values.

Signed-off-by: Gerson Fernando Budke <[email protected]>
The IEEE 802.15.4 examples uses NET_CONFIG_IEEE802154_DEV_NAME config
to determine the default device name to bind. This allows user define
at DT node label the iface name. The default for all examples will be
the first interface instance. This ensure on a multi band board with
2.4G and Sub-Giga radios that the first defined radio will be used.
User can switch to the second radio definition by simply disabling the
first one.

Since this configuration is applied at Kconfig, changes obligate user
to rebuild all the application. This ensure that cache will not affect
expected behaviour.

Signed-off-by: Gerson Fernando Budke <[email protected]>
@nandojve nandojve force-pushed the kconfig_search_dt_by_compat branch from 1726408 to e40a2f4 Compare March 4, 2020 23:26
@nandojve
Copy link
Member Author

Since there isn't movement here I'm closing.

@nandojve nandojve closed this Mar 13, 2020
@nandojve nandojve deleted the kconfig_search_dt_by_compat branch April 28, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Tooling PR modifies or adds a Device Tree tooling area: Kconfig
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants