Re: [PATCH] dt-bindings: interrupt-controller: Convert fsl,mpic-msi to YAML

[J. Neuschäfer]

On Fri, Apr 04, 2025 at 06:06:39PM +0100, Conor Dooley wrote:
> On Thu, Apr 03, 2025 at 07:38:00PM +0200, J. Neuschäfer via B4 Relay wrote:
> > From: "J. Neuschäfer" <j.ne@xxxxxxxxxx>
> > 
> > As part of a larger effort to bring various PowerPC-related bindings
> > into the YAML world, this patch converts msi-pic.txt to YAML and moves
> > it into the bindings/interrupt-controller/ directory. The conversion may
> > necessarily be a bit hard to read because the binding is quite verbose.
> > 
> > Signed-off-by: J. Neuschäfer <j.ne@xxxxxxxxxx>
> > ---
> >  .../interrupt-controller/fsl,mpic-msi.yaml         | 141 +++++++++++++++++++++
> >  .../devicetree/bindings/powerpc/fsl/msi-pic.txt    | 111 ----------------
> >  2 files changed, 141 insertions(+), 111 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,mpic-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,mpic-msi.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..99a98864bd10c5e5b67112c0149fe123b51ca26f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,mpic-msi.yaml
> > @@ -0,0 +1,141 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/fsl,mpic-msi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale MSI interrupt controller
> > +
> > +description:
> 
> I think you want some sort of chomping operator here to preserve
> formatting.

Sounds good, will do.

> 
> > +  The Freescale hypervisor and msi-address-64
> > +  -------------------------------------------
> > +
> > +  Normally, PCI devices have access to all of CCSR via an ATMU mapping.  The
> > +  Freescale MSI driver calculates the address of MSIIR (in the MSI register
> > +  block) and sets that address as the MSI message address.
[...]
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - enum:
> > +          - fsl,mpic-msi
> > +          - fsl,mpic-msi-v4.3
> > +          - fsl,ipic-msi
> > +          - fsl,vmpic-msi
> > +          - fsl,vmpic-msi-v4.3
> > +      - items:
> > +          - enum:
> > +              - fsl,mpc8572-msi
> > +              - fsl,mpc8610-msi
> > +              - fsl,mpc8641-msi
> > +          - const: fsl,mpic-msi
> > +    description:
> 
> > +      compatible list, may contain one or two entries The first is
> > +      "fsl,CHIP-msi", where CHIP is the processor(mpc8610, mpc8572, etc.) and
> > +      the second is "fsl,mpic-msi" or "fsl,ipic-msi" or "fsl,mpic-msi-v4.3"
> > +      depending on the parent type and version. 
> 
> I think this just dupes the compatible list and should be dropped.

Sounds good.

> 
> > If mpic version is 4.3, the
> > +      number of MSI registers is increased to 16, MSIIR1 is provided to access
> > +      these 16 registers, and compatible "fsl,mpic-msi-v4.3" should be used.
> 
> This part is kinda stating the obvious I /think/ but might not be for
> odd reason?

For part of it (using fsl,mpic-msi-v4.3 for version 4.3) yes, but the
other part, the relation between the version and the register (MSIIR vs.
MSIIR1) and number of shared interrupts (8 vs. 16) is only obvious for
readers that have read the hardware reference manuals in detail.

Since it's a difference that essentially depends on the value of
"compatible", I'll rewrite it as an "if/then/else" block in the schema.

> 
> > +      The first entry is optional; the second entry is required.
> 
> I think this part is confusing and should be dropped.

I'll drop it, since it's also redundant with the "oneOf: ..." list for
the compatible property.

> 
> > +
> > +  reg:
> > +    minItems: 1
> > +    items:
> > +      - description: Address and length of the shared message interrupt
> > +          register set
> 
> > +      - description: Address of aliased MSIIR or MSIIR1 register for platforms
> > +          that have such an alias. If using MSIIR1, the second region must be
> > +          added because different MSI group has different MSIIR1 offset.
> 
> This part is based on platform, so should it not have an if/then/else
> below restricting it to the correct platforms?

Good point, I'll do that.

> 
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 16
> > +    description:
> > +      Each one of the interrupts here is one entry per 32 MSIs, and routed to
> > +      the host interrupt controller. The interrupts should be set as edge
> > +      sensitive. If msi-available-ranges is present, only the interrupts that
> > +      correspond to available ranges shall be present.
> > +
> > +  msi-available-ranges:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    items:
> > +      items:
> > +        - description: First MSI interrupt in this range
> > +        - description: Number of MSI interrupts in this range
> > +    description:
> 
> > +      Use <start count> style section to define which MSI interrupt can be used
> > +      in the 256 msi interrupts.
> 
> I think this dupes information in the items list in a more confusing
> manner.

I'll trim it down.

> 
> > This property is optional, without this, all
> > +      the MSI interrupts can be used.  Each available range must begin and end
> > +      on a multiple of 32 (i.e.  no splitting an individual MSI register or the
> > +      associated PIC interrupt).
> 
> > MPIC v4.3 does not support this property
> > +      because the 32 interrupts of an individual register are not continuous
> > +      when using MSIIR1.
> 
> Sounds like another if/then/else should restrict this too.

I'll move it under if/else.


> Rest seems fine :)
> 
> Cheers,
> Conor.


Thank you for your review!

J. Neuschäfer