devicetree: Remove special-casing for clocks, treating them like other phandle-array props #21506
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ulfalizer
requested review from
alexanderwachter,
andrewboie,
arnopo,
cybertale,
erwango,
henrikbrixandersen,
karstenkoenig,
MaureenHelm,
mnkp,
pabigot,
rsalveti and
tbursztyka
as code owners
ulfalizer
force-pushed
the
clock-consistency
branch
from
cca94e9
to
0006255
Compare
ulfalizer
force-pushed
the
clock-consistency
branch
from
0006255
to
e185573
Compare
|
The script change and the C code changes need to go in together. I just split it into multiple commits to make it easier to review. Maybe this could go in as a merge commit. I could squash all of the commits together too, though it makes it a bit hard to understand. (Deleted a spammy message from zephyrbot. Was just a bunch of line-over-80-chars warnings.) |
|
All checks passed. checkpatch (informational only, not a failure)Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
|
While I generally think this is good simplification, I'm afraid it will break many out-of-tree drivers/bindings? |
|
@henrikbrixandersen I think the current devicetree output is in a bad enough shape that it's not worth preserving perfect backwards compatibility everywhere, or we'll never get anywhere. Once things have been streamlined a bit, we could be more careful. |
henrikbrixandersen
approved these changes
Dec 20, 2019
ulfalizer
force-pushed
the
clock-consistency
branch
from
e185573
to
83ae09d
Compare
|
@henrikbrixandersen
I also moved out the |
"could". And this is exactly why I get so frustrated with zephyr (@carlescufi). "The current solution is bad, so let's keep breaking it for everybody incrementally to fix the immediate problem until we finally figure out what we want to do." No plan, just patching. Even leaving the old one in as No, I don't have a better solution. But this is not consistent with Zephyr being "a quality code base with good engineering practices". |
carlescufi
requested changes
Dec 20, 2019
There was a problem hiding this comment.
I think the current devicetree output is in a bad enough shape that it's not worth preserving perfect backwards compatibility everywhere, or we'll never get anywhere. Once things have been streamlined a bit, we could be more careful.
"could".
And this is exactly why I get so frustrated with zephyr (@carlescufi). "The current solution is bad, so let's keep breaking it for everybody incrementally to fix the immediate problem until we finally figure out what we want to do." No plan, just patching.
Even leaving the old one in as
deprecatedcauses pain, since it produces build warnings that have to be fixed in pending PRs. Which happened with theparent/child-busrework.No, I don't have a better solution. But this is not consistent with Zephyr being "a quality code base with good engineering practices".
In this case, I completely agree with @pabigot. Before we move forward with a change like this, we need to hear from the Device Tree maintainer, in this case @galak. Only after we know what the plan is mid and long term we should introduce breaking changes. That doesn't mean this PR is not going in the right direction, but we need to decide if this is the final direction before we merge it.
ulfalizer
force-pushed
the
clock-consistency
branch
from
a72d801
to
97120d8
Compare
Rebased now. Will do a check for newly-added stuff a bit later. Marking DNM for now. |
galak
added
area: Devicetree Tooling
ulfalizer
force-pushed
the
clock-consistency
branch
from
97120d8
to
34712df
Compare
ulfalizer
force-pushed
the
clock-consistency
branch
from
34712df
to
df5556b
Compare
ulfalizer
force-pushed
the
clock-consistency
branch
from
df5556b
to
e4c8ded
Compare
Should be fine again now. |
galak
pushed a commit
to ulfalizer/zephyr
that referenced
this pull request
Jan 31, 2020
Works like __DEPRECATED_MACRO with a custom message. Can do this for
example:
#define FOO __WARN("Please use BAR instead") ...
Implement __DEPRECATED_MACRO with __WARN().
Useful for zephyrproject-rtos#21506.
Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
nashif
pushed a commit
that referenced
this pull request
Feb 1, 2020
Works like __DEPRECATED_MACRO with a custom message. Can do this for
example:
#define FOO __WARN("Please use BAR instead") ...
Implement __DEPRECATED_MACRO with __WARN().
Useful for #21506.
Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Remove almost all special-casing for 'clocks = ...' properties and
handle them with the generic phandle-array code. The following commits
will make the required changes to C and header files.
These output inconsistencies were fixed to able to reuse the
phandle-array code:
- Properties like 'pwms = ...' and '*-gpios = ...' generate identifiers
with 'PWMS' and 'GPIOS' (note the -S), while 'clocks' generated
'CLOCK'.
- A non-indexed '*_CLOCK_<clock-cells entry>' identifier (as opposed to
'*_CLOCK_<clock-cells entry>_0') was always generated, regardless of
how many clocks there were. Properties like 'pwms = ...' only
generate a non-indexed identifier if there's a single entry in the
phandle-array.
(In practice, no 'clocks = ...' has more than one entry in Zephyr.)
The only special-casing that remains is for 'fixed-clock':
write_clocks() is now write_freq(), which writes the 'clock-frequency'
from any controller in 'clocks' that's compatible with 'fixed-clock'.
Besides reducing code duplication and making things less surprising and
easier to understand, this change has two nice side effects:
- 'description:' texts now show up for clocks now show up in the output
- Static initializers like
#define ..._CLOCKS {"SIM", 0, 4152, 20}
now get generated.
Also add a check for 'clocks' being a phandle-array in write_freq().
Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Add back generation of the the non-phandle-array-consistent clock defines, but generate deprecation warnings for them if they're used, with a message saying to use *_CLOCKS_* instead of *_CLOCK_*. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Copy-pasted around. Not used anywhere in the code. Equivalent to
*_CLOCK_{BITS,BUS} without an index.
I'm adapting all clock code to the preceding gen_defines.py change, and
I'd rather remove unused identifiers than fix them.
Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Correct as long as there's a single clock in 'clocks = ...', which there always is at the moment. Makes the code compatible with write_phandle_val_list_entry(). Adapting all clock code to the preceding gen_defines.py change. Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
gen_defines.py generated macros with 'PWMS' and 'GPIOS' in them for 'pwms = ...' and '*-gpios = ...', but 'clocks = ...' generates 'CLOCK', with no -S. Replace 'CLOCK' with 'CLOCKS' to fix the inconsistency. Adapting all clock code to the preceding gen_defines.py change. I was careful to not change any macros defined in dts_fixup.h files (which are currently also prefixed with 'DT_'). Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
galak
force-pushed
the
clock-consistency
branch
from
e4c8ded
to
698788a
Compare
|
@erwango can you take a look. |
erwango
approved these changes
Feb 13, 2020
|
Closing this as lot of this change will/is happening with the conversion to the new DT macros. |
jadonk
pushed a commit
to beagleboard/zephyr
that referenced
this pull request
Aug 1, 2023
Works like __DEPRECATED_MACRO with a custom message. Can do this for
example:
#define FOO __WARN("Please use BAR instead") ...
Implement __DEPRECATED_MACRO with __WARN().
Useful for zephyrproject-rtos#21506.
Signed-off-by: Ulf Magnusson <Ulf.Magnusson@nordicsemi.no>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove almost all special-casing for
clocks = ...properties andhandle them with the generic phandle-array code. The commits
below make the required changes to C and header files.
These output inconsistencies were fixed to able to reuse the
phandle-array code:
Properties like
pwms = ...and*-gpios = ...generate identifierswith
PWMSandGPIOS(note the -S), whileclocksgeneratedCLOCK.A non-indexed
*_CLOCK_<clock-cells entry>identifier (as opposed to*_CLOCK_<clock-cells entry>_0) was always generated, regardless ofhow many clocks there were. Properties like
pwms = ...onlygenerate a non-indexed identifier if there's a single entry in the
phandle-array.
(In practice, no
clocks = ...has more than one entry in Zephyr.)The only special-casing that remains is for
fixed-clock:write_clocks()is nowwrite_freq(), which writes theclock-frequencyfrom any controller in
clocksthat's compatible withfixed-clock.Besides reducing code duplication and making things less surprising and
easier to understand, this change has two nice side effects:
description:texts now show up for clocks now show up in the outputStatic initializers like
now get generated.
Also add a check for
clocksbeing a phandle-array inwrite_freq().Using the old macros now generates a warnings saying to use
*_CLOCKS_*' instead ofCLOCK`, via these commits:Fixes in C code: