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

devicetree: bindings: add top-level interrupt-source key #56788

Conversation

mbolivar-nordic
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic commented Apr 12, 2023


Note: this is prep work for system devicetree ( #51830 ) which is standalone and I believe is mergeable now.

We want to be able to re-use the same binding for the same peripheral IP blocks regardless of whether they appear in a single- or multi-core SoC. Switching to interrupt-source in Nordic bindings enables this.


Zephyr devicetree bindings support either the 'interrupts' or the
'interrupts-extended' method of specifying interrupts. These
properties are standard and defined in the Devicetree Specification.

The 'interrupts' property can be treated as a special case of the
'interrupts-extended' property; you can always write something like
this:

  node {
          interrupt-parent = <&parent>;
          interrupts = <1>;
  };

as this instead:

  node {
          interrupts-extended = <&parent 1>;
  };

The 'interrupts-extended' property, however, can be used when the node
in question is hooked up to more than one interrupt controller, and
can contain multiple elements, one per interrupt domain that the node
generates interrupts in.

Therefore, it is sensible semantically to allow an
interrupt-generating devicetree node to use either
'interrupts-extended' or 'interrupts' to specify its generated
interrupts, at the option of the devicetree author, and we can almost
do that today.

The missing piece is that we can't require in the binding that either
'interrupts' or 'interrupts-extended' is set: we can only require
individual properties; we can't say "at least one of these is
required".

You could write some BUILD_ASSERT()s to do this in the driver, but it
would be nicer to have this in the binding so the error messages are
easier to read. The extra effort seems worth it since this is a
special case where the properties are core to the specification
itself.

To enable this, add a top-level 'interrupt-source:' key to the
bindings syntax which, if true, indicates that one of 'interrupts' or
'interrupts-extended' must be set:

compatible: foo
interrupt-source: true
[...]

The use of the token 'prop' in a few places related to how we handle
devicetree binding YAML files could be misleading. It seems to
indicate that the values being dealt with are property names or
something to do with properties, but in fact they are just arbitrary
dict keys at this point in the code.

Making 'check_required' a positional argument and then being
inconsistent about passing it as one or a keyword argument seems
worth cleaning up.

Refactor the names so that they are clearer about this, and make
check_required a keyword argument.

No functional changes expected.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This extension will make it easier to write a test for an upcoming
feature.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Zephyr devicetree bindings support either the 'interrupts' or the
'interrupts-extended' method of specifying interrupts. These
properties are standard and defined in the Devicetree Specification.

The 'interrupts' property can be treated as a special case of the
'interrupts-extended' property; you can always write something like
this:

  node {
          interrupt-parent = <&parent>;
          interrupts = <1>;
  };

as this instead:

  node {
          interrupts-extended = <&parent 1>;
  };

The 'interrupts-extended' property, however, can be used when the node
in question is hooked up to more than one interrupt controller, and
can contain multiple elements, one per interrupt domain that the node
generates interrupts in.

Therefore, it is sensible semantically to allow an
interrupt-generating devicetree node to use either
'interrupts-extended' or 'interrupts' to specify its generated
interrupts, at the option of the devicetree author, and we can almost
do that today.

The missing piece is that we can't require in the binding that either
'interrupts' or 'interrupts-extended' is set: we can only require
individual properties; we can't say "at least one of these is
required".

You could write some BUILD_ASSERT()s to do this in the driver, but it
would be nicer to have this in the binding so the error messages are
easier to read. The extra effort seems worth it since this is a
special case where the properties are core to the specification
itself.

To enable this, add a top-level 'interrupt-source:' key to the
bindings syntax which, if true, indicates that one of 'interrupts' or
'interrupts-extended' must be set.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This new feature requires a bit of special handling since it affects
multiple different properties.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Adapt devicetree bindings for Nordic SoCs to use the
'interrupt-source' feature which was recently added to the devicetree
bindings syntax.

This paves the way for using them in system devicetree, for example in
nRF54H20, without affecting backwards compatibility for existing use
cases. We update all peripherals just to be consistent within our
vendor bindings, while leaving it up to other SoC maintainers to
decide what is best for their use cases.

(The reason why this is useful for system devicetree support is that
in a system devicetree, a single peripheral node may need to be able
to express its connections to multiple interrupt controllers, such as
the two independent NVICs in an SoC with two separate Arm Cortex-M
cores in an AMP configuration.)

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

I'd rather see if we can come up with a more generic way of describing the requirement instead of adding a new special case keyword.

Also, if we do go done the path of interrupt-source, is there any case in which either form should not be supported? (ie, thinking of changing base.yaml).

@galak
Copy link
Collaborator

galak commented Apr 12, 2023

Is there anything we can take / model off of on how this might be handled in dt-schema?

@galak
Copy link
Collaborator

galak commented Apr 12, 2023

Looking at dtschema/schemas/interrupts.yaml , seems we could possibly introduce a dependencies section:

dependencies:
    oneOf:
      - required:
          - interrupts
      - required:
          - interrupts-extended

@mbolivar-nordic
Copy link
Contributor Author

I'd rather see if we can come up with a more generic way of describing the requirement instead of adding a new special case keyword.

Given that we want to go to schema eventually, is it really worth implementing this part of it now? Do you have a concrete other use case for this level of reimplementation in mind?

@mbolivar-nordic
Copy link
Contributor Author

Looking at dtschema/schemas/interrupts.yaml , seems we could possibly introduce a dependencies section:

This syntax is pretty heavyweight to add just for this one thing and it would be harder to handle in gen_devicetree_rest. I was really hoping we could keep this minimal for now instead of start going down the road of reimplementing dt-schema when we know we'd like to transition eventually anyway

@galak
Copy link
Collaborator

galak commented Apr 12, 2023

This syntax is pretty heavyweight to add just for this one thing and it would be harder to handle in gen_devicetree_rest. I was really hoping we could keep this minimal for now instead of start going down the road of reimplementing dt-schema when we know we'd like to transition eventually anyway

Let's close if we think this needs to exist anywhere outside of base.yaml. I'm more inclined to be ok with doing interrupt-source if it really just ends up in one place.

@mbolivar-nordic
Copy link
Contributor Author

At least for system DT, I don't know of any other place where I need to extend the bindings syntax so far. I might end up extending the compound type a bit to describe the contents of complicated properties like cpus in an execution domain, but that's all about being able to go beyond phandle-array in our current bindings, and I'm not even sure I need that.

TBH I would probably just drop this PR and add BUILD_ASSERT()s to the relevant drivers to make sure at least one property is defined rather than start re-implementing the more complicated parts of dt-schema one at a time.

@henrikbrixandersen
Copy link
Member

TBH I would probably just drop this PR and add BUILD_ASSERT()s to the relevant drivers to make sure at least one property is defined rather than start re-implementing the more complicated parts of dt-schema one at a time.

I think that makes sense for now.

@mbolivar-nordic
Copy link
Contributor Author

Not pursuing this further given feedback from @galak and @henrikbrixandersen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants