Re: [PATCH v3 08/11] dt-bindings: interrupt-controller: Add RISC-V advanced PLIC

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3906 bytes --]

Hey Anup,

On Mon, May 08, 2023 at 07:58:39PM +0530, Anup Patel wrote:
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 16384
> +    description:
> +      Given APLIC domain directly injects external interrupts to a set of
> +      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv,cpu-intc
> +      node, which has a riscv node (i.e. RISC-V HART) as parent.

Same nit here, s/riscv node/cpu node/?

> +
> +  msi-parent:
> +    description:
> +      Given APLIC domain forwards wired interrupts as MSIs to a AIA incoming
> +      message signaled interrupt controller (IMSIC). If both "msi-parent" and
> +      "interrupts-extended" properties are present then it means the APLIC
> +      domain supports both MSI mode and Direct mode in HW. In this case, the
> +      APLIC driver has to choose between MSI mode or Direct mode.
> +
> +  riscv,num-sources:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 1023
> +    description:
> +      Specifies the number of wired interrupt sources supported by this
> +      APLIC domain.

Rob asked:
| We don't normally need to how many interrupts, why here?

But I do not see an answer to that on lore.

> +
> +  riscv,children:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    maxItems: 1024
> +    items:
> +      maxItems: 1
> +    description:
> +      A list of child APLIC domains for the given APLIC domain. Each child
> +      APLIC domain is assigned a child index in increasing order, with the
> +      first child APLIC domain assigned child index 0. The APLIC domain child
> +      index is used by firmware to delegate interrupts from the given APLIC
> +      domain to a particular child APLIC domain.
> +
> +  riscv,delegate:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    maxItems: 1024
> +    items:
> +      items:
> +        - description: child APLIC domain phandle
> +        - description: first interrupt number of this APLIC domain (inclusive)
> +        - description: last interrupt number of this APLIC domain (inclusive)
> +    description:
> +      A interrupt delegation list where each entry is a triple consisting of
> +      child APLIC domain phandle, first interrupt number of this APLIC domain,
> +      and last interrupt number of this APLIC domain. Firmware must configure
> +      interrupt delegation registers based on interrupt delegation list.

I read back Rob's comments on this from last time. He said:
| The node's domain it delegating its interrupts to the child domain or 
| the other way around? The interrupt numbers here are this domain's or 
| the child's?

IMO it's ambiguous from the binding description whether the numbers
refer to the "first interrupt in the parent domain that is delegated
to the child" or to numbering of the interrupts within the child domain.
"This" can be quite an ambiguous word, and it's not totally obvious
whether the "this" refers to the "child domain" earlier in the sentence.

I also do not not think you have answered his question about the
directionality of the delegation either (it should just be a copy-paste
from riscv,children, no?).

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - riscv,num-sources

btw, do we need something like:

anyOf:
  - required:
      - interrupts-extended
  - required:
      - msi-parent

& hopefully I didn't previously ask this one:
dependencies:
  riscv,delegate: [ riscv,children ]

As an aside, I still think "riscv,delegate" looks strange here alongside
"riscv,children" since "delegate" is singular and "children" is plural.
The plural would be "delegates", but "delegation" would also fit better
than "delegate".

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]