2022-04-26 07:12:27

by Michael Walle

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

Convert the fsl,ls-extirq binding to the new YAML format.

In contrast to the original binding documentation, there are three
compatibles which are used in their corresponding device trees which
have a specific compatible and the (already documented) fallback
compatible:
- "fsl,ls1046a-extirq", "fsl,ls1043a-extirq"
- "fsl,ls2080a-extirq", "fsl,ls1088a-extirq"
- "fsl,lx2160a-extirq", "fsl,ls1088a-extirq"

Signed-off-by: Michael Walle <[email protected]>
---
changes since v1:
- new patch, because it's reference in patch 2/2

.../interrupt-controller/fsl,ls-extirq.txt | 53 -----------
.../interrupt-controller/fsl,ls-extirq.yaml | 88 +++++++++++++++++++
2 files changed, 88 insertions(+), 53 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
deleted file mode 100644
index 4d47df1a5c91..000000000000
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
+++ /dev/null
@@ -1,53 +0,0 @@
-* Freescale Layerscape external IRQs
-
-Some Layerscape SOCs (LS1021A, LS1043A, LS1046A
-LS1088A, LS208xA, LX216xA) support inverting
-the polarity of certain external interrupt lines.
-
-The device node must be a child of the node representing the
-Supplemental Configuration Unit (SCFG).
-
-Required properties:
-- compatible: should be "fsl,<soc-name>-extirq", e.g. "fsl,ls1021a-extirq".
- "fsl,ls1043a-extirq": for LS1043A, LS1046A.
- "fsl,ls1088a-extirq": for LS1088A, LS208xA, LX216xA.
-- #interrupt-cells: Must be 2. The first element is the index of the
- external interrupt line. The second element is the trigger type.
-- #address-cells: Must be 0.
-- interrupt-controller: Identifies the node as an interrupt controller
-- reg: Specifies the Interrupt Polarity Control Register (INTPCR) in
- the SCFG or the External Interrupt Control Register (IRQCR) in
- the ISC.
-- interrupt-map: Specifies the mapping from external interrupts to GIC
- interrupts.
-- interrupt-map-mask: Must be <0xffffffff 0>.
-
-Example:
- scfg: scfg@1570000 {
- compatible = "fsl,ls1021a-scfg", "syscon";
- reg = <0x0 0x1570000 0x0 0x10000>;
- big-endian;
- #address-cells = <1>;
- #size-cells = <1>;
- ranges = <0x0 0x0 0x1570000 0x10000>;
-
- extirq: interrupt-controller@1ac {
- compatible = "fsl,ls1021a-extirq";
- #interrupt-cells = <2>;
- #address-cells = <0>;
- interrupt-controller;
- reg = <0x1ac 4>;
- interrupt-map =
- <0 0 &gic GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
- <1 0 &gic GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
- <2 0 &gic GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
- <3 0 &gic GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>,
- <4 0 &gic GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
- <5 0 &gic GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-map-mask = <0xffffffff 0x0>;
- };
- };
-
-
- interrupts-extended = <&gic GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>,
- <&extirq 1 IRQ_TYPE_LEVEL_LOW>;
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 <[email protected]>
+ - Li Yang <[email protected]>
+
+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#
+
+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:
+ description: Specifies the mapping from external interrupts to GIC interrupts.
+
+ interrupt-map-mask:
+ items:
+ - const: 0xffffffff
+ - const: 0
+
+required:
+ - compatible
+ - '#interrupt-cells'
+ - '#address-cells'
+ - interrupt-controller
+ - reg
+ - interrupt-map
+ - interrupt-map-mask
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ interrupt-controller@1ac {
+ compatible = "fsl,ls1021a-extirq";
+ #interrupt-cells = <2>;
+ #address-cells = <0>;
+ interrupt-controller;
+ reg = <0x1ac 4>;
+ interrupt-map =
+ <0 0 &gic GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
+ <1 0 &gic GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
+ <2 0 &gic GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
+ <3 0 &gic GIC_SPI 167 IRQ_TYPE_LEVEL_HIGH>,
+ <4 0 &gic GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
+ <5 0 &gic GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-map-mask = <0xffffffff 0x0>;
+ };
--
2.30.2


2022-04-26 09:25:39

by Krzysztof Kozlowski

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

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 <[email protected]>
> + - Li Yang <[email protected]>
> +
> +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%[email protected]/

This device does not have children, so the interrupt-controller schema
should be probably skipped.


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

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


Best regards,
Krzysztof

2022-04-26 12:40:12

by Krzysztof Kozlowski

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

On 25/04/2022 23:58, Michael Walle wrote:
>>> + 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


Works for me (in different schema)... maybe update your dtschema?

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

Yes, the child (first) interrupt specifier. Since address-cells are 0,
this will be bit-AND with [0-5].

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

Yes.

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


Exactly like this, looks good. Thank you.


Best regards,
Krzysztof

2022-04-26 14:54:17

by Michael Walle

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

Am 2022-04-26 08:53, schrieb Krzysztof Kozlowski:
> On 25/04/2022 23:58, Michael Walle wrote:
>>>> + 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
>
>
> Works for me (in different schema)... maybe update your dtschema?

Just updated to the latest one. But I'm still getting the same errors.

$ dt-validate -V
2022.4

/home/mwalle/repos/b-linux-arm64/arch/arm64/boot/dts/freescale/fsl-ls1088a-qds.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
From schema:
/home/mwalle/repos/linux-mw/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml

How is the length of one entry calculated?

-michael

2022-04-26 19:00:48

by Michael Walle

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

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 <[email protected]>
>> + - Li Yang <[email protected]>
>> +
>> +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%[email protected]/
>
> 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

2022-04-27 08:00:18

by Krzysztof Kozlowski

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

On 26/04/2022 09:28, Michael Walle wrote:
> Am 2022-04-26 08:53, schrieb Krzysztof Kozlowski:
>> On 25/04/2022 23:58, Michael Walle wrote:
>>>>> + 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
>>
>>
>> Works for me (in different schema)... maybe update your dtschema?
>
> Just updated to the latest one. But I'm still getting the same errors.
>
> $ dt-validate -V
> 2022.4
>
> /home/mwalle/repos/b-linux-arm64/arch/arm64/boot/dts/freescale/fsl-ls1088a-qds.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
> From schema:
> /home/mwalle/repos/linux-mw/Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml
>
> How is the length of one entry calculated?

If you add maxItems to your original v2 binding example, it works. If
you replace your example with ls1088a and use maxItems:12, it works.

So maybe something is wrong in your modified patch (which we do not have
so we cannot test it)?

Best regards,
Krzysztof