Skip to content

Refactor and fix Cortex-R device tree #19696

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

Merged
merged 2 commits into from
Oct 11, 2019

Conversation

stephanosio
Copy link
Member

Refactor and fix Cortex-R device tree

This commit adds device bindings for Cortex-R4(F) and Cortex-R5(F).

These were supposed to be added during the initial development of
Cortex-R port, but it was not due to an incorrect device tree
specification.
1. Replace the non-existent CPU device binding ("Cortex-R") specified
   by the CPU node with a proper one.

2. Relocate CPU node declaration to SoC dtsi:

  The CPU node should be declared in the SoC dtsi because the core
  type is SoC-dependent. In fact, this is exactly how it is done in
  the Cortex-M port.

3. Remove core_intc (supposedly Cortex-R VIC):

  Unlike the NVIC of Cortex-M, the VIC of Cortex-R is not a true
 interrupt controller in the conventional sense and merely acts as
 a CPU input port for aggregated interrupt request and vector index
 signals. For this reason, there is no point in declaring it in the
 device tree and specifying it as an interrupt parent. All SoCs
 incorporating Cortex-R implement a separate true interrupt
 controller (for instance, GIC for Zynq MPSoC and VIM for Hercules).

This PR is part of the preparations for #19644.

@stephanosio stephanosio requested a review from galak as a code owner October 8, 2019 17:08
@stephanosio stephanosio mentioned this pull request Oct 8, 2019
32 tasks
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 8, 2019

All checks are passing now.

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

@ioannisg ioannisg added the area: ARM ARM (32-bit) Architecture label Oct 9, 2019
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@ioannisg ioannisg added this to the v2.1.0 milestone Oct 9, 2019
@ioannisg ioannisg requested a review from ulfalizer October 9, 2019 14:09
title: ARM Cortex-R5F CPU

description: >
This binding gives a base representation for ARM Cortex-R5F CPU.
Copy link
Collaborator

@ulfalizer ulfalizer Oct 9, 2019

Choose a reason for hiding this comment

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

Could shorten to just be the same thing as the title.

It shows up as a comment above the generated #defines, so it looks less confusing too.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the other bindings seem to use This binding gives a base representation ... or a similar long form in the description.

Is there a plan to change this format in a near future? If not, I believe, at least for the sake of uniformity, it should follow the form used by all the other bindings.

Maybe it can be updated later when, if ever, such a change is implemented across the repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a "fight the flow" kind of guy personally ;). Not super important though. Might do a mass-fixup later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't even know what "base" is supposed to mean there to be honest. It's not like there are other bindings for it.

I suspect it meant something wherever it was added originally, and then it was copied from there to everywhere. Trying to get rid of mystery stuff like that long-term, so that it's obvious what everything is doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't even know what "base" is supposed to mean there to be honest. It's not like there are other bindings for it.

Fair point. It's not like this is a "base" binding that is included by other bindings (as it is done to cpu.yaml, for instance). I will update the description to a more generic one that goes like This is a representation of ....

I still think the description field deserves more than just the title though; after all, that is the whole point of having a description field at all. At the same time, I do understand that it is rather pointless to have a description that simply states the very same thing as the title and does not enrich developer understanding of a specific binding as done in here.

The real problem here is that the description field must be present, and you are forced to fill it out regardless of whether you actually need it or not. Removing an unneeded description field will result in devicetree error: missing 'description' property in *.

Maybe, making the description field optional and allowing it to be specified only when it is required would be a more sensible approach to this problem, as it is done in Linux.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been thinking of removing title instead, because it's unused now.

I still think the description field deserves more than just the title though; after all, that is the whole point of having a description field at all. At the same time, I do understand that it is rather pointless to have a description that simply states the very same thing as the title and does not enrich developer understanding of a specific binding as done in here.

Yeah, having a long detailed description is nice. "This is a representation of ..." feels like boilerplate though.

If you have description: Foo device running at five MHz with an antenna, then description: This is a representation of a Foo device running at five MHz with an antenna doesn't make it better. :)

Copy link
Member Author

@stephanosio stephanosio Oct 10, 2019

Choose a reason for hiding this comment

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

I've been thinking of removing title instead, because it's unused now.

Just a suggestion- how about keeping the title field and making the description field optional? In terms of generated dts header, maybe the script can insert title in place of "Binding description" if the optional description is unspecified (or maybe binding description isn't really needed there, just title should enough to give a reasonably clear idea as to what the binding represents; after all, 'title' really is just a short summary description whereas 'description' is a long detailed description).

The rationale here is that title is still a nice thing to have even if there is a long detailed description because it provides a quick and consistent way to refer to something.

For instance, if we have a binding like below, it would be easier to refer to it as "Atmel SAM0 32-bit Basic Timer binding" than "the binding for Atmel SAM0 timer counter operating in 32-bit wide mode." Of course, you could refer to it as "atmel,sam0-tc32.yaml" instead, but that sounds rather cryptic.

(atmel,sam0-tc32.yaml)

title: Atmel SAM0 32-bit Basic Timer

description: >
    This binding gives a base representation of the Atmel SAM0 timer
    counter (TC) operating in 32-bit wide mode.

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

LGTM

@carlescufi carlescufi requested a review from MaureenHelm October 9, 2019 20:06
This commit adds device bindings for Cortex-R4(F) and Cortex-R5(F).

These were supposed to be added during the initial development of
Cortex-R port, but it was not due to an incorrect device tree
specification.

Signed-off-by: Stephanos Ioannidis <[email protected]>
1. Replace the non-existent CPU device binding ("Cortex-R") specified
   by the CPU node with a proper one.

2. Relocate CPU node declaration to SoC dtsi:

  The CPU node should be declared in the SoC dtsi because the core
  type is SoC-dependent. In fact, this is exactly how it is done in
  the Cortex-M port.

3. Remove core_intc (supposedly Cortex-R VIC):

  Unlike the NVIC of Cortex-M, the VIC of Cortex-R is not a true
 interrupt controller in the conventional sense and merely acts as
 a CPU input port for aggregated interrupt request and vector index
 signals. For this reason, there is no point in declaring it in the
 device tree and specifying it as an interrupt parent. All SoCs
 incorporating Cortex-R implement a separate true interrupt
 controller (for instance, GIC for Zynq MPSoC and VIM for Hercules).

Signed-off-by: Stephanos Ioannidis <[email protected]>
@ioannisg
Copy link
Member

@ulfalizer, I wonder if you have got more comments/feedback on this PR? :)

@ioannisg ioannisg merged commit e87ccbc into zephyrproject-rtos:master Oct 11, 2019
@stephanosio stephanosio deleted the cortex_r_dts_fix branch October 17, 2019 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Devicetree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants