Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: fsl,ls-extirq: convert to YAML

[Michael Walle]

Am 2022-04-25 20:36, schrieb Krzysztof Kozlowski:
> On 25/04/2022 16:02, Michael Walle wrote:
> > Convert the fsl,ls-extirq binding to the new YAML format.
>
> (...)
>
> > diff --git 
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
> > new file mode 100644
> > index 000000000000..39d120ad7549
> > --- /dev/null
> > +++ 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
> > @@ -0,0 +1,88 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > http://devicetree.org/schemas/interrupt-controller/fsl,ls-extirq.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale Layerscape External Interrupt Controller
> > +
> > +maintainers:
> > +  - Shawn Guo <shawnguo@xxxxxxxxxx>
> > +  - Li Yang <leoyang.li@xxxxxxx>
> > +
> > +description: |
> > +  Some Layerscape SOCs (LS1021A, LS1043A, LS1046A LS1088A, LS208xA,
> > +  LX216xA) support inverting the polarity of certain external 
> > interrupt
> > +  lines.
> > +
> > +allOf:
> > +  - $ref: /schemas/interrupt-controller.yaml#
>
> I have doubts whether this is here applicable. See also Rob's comment:
> https://lore.kernel.org/all/YjjJpxLWJ%2FYOe0OX@xxxxxxxxxxxxxxxxxx/
>
> This device does not have children, so the interrupt-controller schema
> should be probably skipped.

ok.

> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - enum:
> > +          - fsl,ls1021a-extirq
> > +          - fsl,ls1043a-extirq
> > +          - fsl,ls1088a-extirq
> > +      - items:
> > +          - enum:
> > +              - fsl,ls1046a-extirq
> > +          - const: fsl,ls1043a-extirq
> > +      - items:
> > +          - enum:
> > +              - fsl,ls2080a-extirq
> > +              - fsl,lx2160a-extirq
> > +          - const: fsl,ls1088a-extirq
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  '#address-cells':
> > +    const: 0
> > +
> > +  interrupt-controller: true
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      Specifies the Interrupt Polarity Control Register (INTPCR) in 
> > the
> > +      SCFG or the External Interrupt Control Register (IRQCR) in the 
> > ISC.
> > +
> > +  interrupt-map:

btw.

minItems: 12
maxItems: 12

Isn't working here, is that expected? The validator seem to get the 
count
of the elements of one tuple wrong.

I.e.
arch/arm64/boot/dts/freescale/fsl-ls2080a-rdb.dtb: 
interrupt-controller@14: interrupt-map: [[0, 0, 1, 0, 0, 4, 1, 0], [1, 
0, 1, 4, 2, 0, 1, 0], [2, 4, 3, 0, 1, 0, 3, 4], [4, 0, 1, 0, 4, 4, 5, 
0], [1, 0, 5, 4, 6, 0, 1, 0], [6, 4, 7, 0, 1, 0, 7, 4], [8, 0, 1, 0, 8, 
4, 9, 0], [1, 0, 9, 4, 10, 0, 1, 0], [10, 4, 11, 0, 1, 0, 11, 4]] is too 
short

> > +    description: Specifies the mapping from external interrupts to 
> > GIC interrupts.
> > +
> > +  interrupt-map-mask:
> > +    items:
> > +      - const: 0xffffffff
>
> This looks highly permissive mask and should be instead defined per
> variant, for example (quickly looking at DTS):
> 0x7 for ls1021
> 0xf for ls1043a and ls1088a

Just that I understand it correctly, the result of the AND with that
mask is then looked up in the interrupt-map (the first entry there)?

> You might need to correct the DTS. Some confirmation from someone with
> datasheet would be good.

According to their datasheets they have the following number of external
IRQs:
- ls1021a has 6,
- ls1043a has 12,
- ls1046a has 12,
- ls1088a has 12,
- ls2080a has 12,
- lx2160a has 12.

That is what I need to confirm, right?

Is there a better way than the following snippet:

properties:
  interrupt-map-mask: true

allOf:
  - if:
      properties:
        compatible:
          contains:
            enum:
              - fsl,ls1021a-extirq
    then:
      properties:
        interrupt-map-mask:
          items:
            - const: 0x7
            - const: 0
  - if:
      properties:
        compatible:
          contains:
            enum:
              - fsl,ls1043a-extirq
              - fsl,ls1046a-extirq
              - fsl,ls1088a-extirq
              - fsl,ls2080a-extirq
              - fsl,lx2160a-extirq
    then:
      properties:
        interrupt-map-mask:
          items:
            - const: 0xf
            - const: 0

-michael