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

Anup Patel
2023-06-13T10:37:45Z

On Wed, May 10, 2023 at 9:11 PM Conor Dooley <conor@kernel.org> wrote:
>
> 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/?

Okay, I will update.

>
> > +
> > + 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.

The APLIC spec defines maximum interrupt sources to be 1023.

>
> > +
> > + 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?

The node's domain is delegating its interrupts to the child domain.

>
> 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?).

For APLIC, the interrupt delegation is always from parent domain
to the child domain.

I will add a statement about this in the description.

>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupt-controller
> > + - "#interrupt-cells"
> > + - riscv,num-sources
>
> btw, do we need something like:
>
> anyOf:
> - required:
> - interrupts-extended
> - required:
> - msi-parent

Okay, I will update.

>
> & 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".

Okay, I will rename it to "riscv,delegation".

>
> Cheers,
> Conor.

Regards,
Anup