diff mbox series

[2/2] dt-bindings: interrupt-controller: Add MStar interrupt controller

Message ID 20200819034231.20726-3-mark-pk.tsai@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Add MStar interrupt controller support | expand

Commit Message

Mark-PK Tsai (蔡沛剛) Aug. 19, 2020, 3:42 a.m. UTC 
Add binding for MStar interrupt controller.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 .../interrupt-controller/mstar,mst-intc.yaml  | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml

Comments

Rob Herring Aug. 25, 2020, 9:48 p.m. UTC | #1 
On Wed, Aug 19, 2020 at 11:42:31AM +0800, Mark-PK Tsai wrote:
> Add binding for MStar interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../interrupt-controller/mstar,mst-intc.yaml  | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> new file mode 100644
> index 000000000000..6e383315e529
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings.

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mstar,mst-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar Interrupt Controller
> +
> +maintainers:
> +  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> +
> +description: |+
> +  MStar, SigmaStar and Mediatek DTV SoCs contain multiple legacy
> +  interrupt controllers that routes interrupts to the GIC.
> +
> +  The HW block exposes a number of interrupt controllers, each
> +  can support up to 64 interrupts.
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#

Drop this. It is applied based on node name matching already.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mstar,mst-intc
> +      - enum:
> +          - mediatek,mt58xx-intc

Normally, the 1st entry would be enum as that's where you'd add new 
compatibles (as the fallback is constant). But if you don't forsee any 
additions, just make both 'const'

> +
> +  interrupt-controller: true
> +
> +  "#address-cells":
> +    enum: [ 0, 1, 2 ]

This would normally be 0 in an interrupt controller. It's only relevant 
if you have an 'interrupt-map' which this is the parent for.

> +  "#size-cells":
> +    enum: [ 1, 2 ]

And this should be dropped.

> +
> +  "#interrupt-cells":
> +    const: 3
> +    description: |
> +      Use the same format as specified by GIC in arm,gic.yaml.

That's odd. You have the same SPI and PPI stuff?

> +
> +  reg:
> +    description: |
> +      Physical base address of the mstar interrupt controller
> +      registers and length of memory mapped region.

Drop this. That's every 'reg' property.

> +    minItems: 1

maxItems is more logical.

> +
> +  mstar,irqs-map-range:
> +    description: |
> +      The range of parent interrupt controller's interrupt lines
> +      that are hardwired to mstar interrupt controller.

Is this <start size> or <start end>?

Really, this should just use 'interrupts' even though that's a bit 
verbose. Or be implied by the compatible string. What's the maximum 
number of parent interrupts?

In any case, we really need to stop having vendor specific properties 
for this.

> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      minItems: 2
> +      maxItems: 2
> +
> +  mstar,intc-no-eoi:
> +    description: |

Don't need '|' if there's no formatting.

> +      Mark this controller has no End Of Interrupt(EOI) implementation.
> +      This is a empty, boolean property.

You can drop this line. The schema says this.

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - mstar,irqs-map-range
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mst_intc0: interrupt-controller@1f2032d0 {
> +      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
> +      interrupt-controller;
> +      #interrupt-cells = <3>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      interrupt-parent = <&gic>;
> +      reg = <0x1f2032d0 0x30>;
> +      mstar,irqs-map-range = <0 63>;

Is 0 a PPI or SPI? This property is making some assumption and wouldn't 
be able to support both types or another parent interrupt controller.

> +    };
> +...
> -- 
> 2.18.0
Mark-PK Tsai (蔡沛剛) Aug. 26, 2020, 7:50 a.m. UTC | #2 
From: Rob Herring <robh@kernel.org>

>> +
>> +  "#interrupt-cells":
>> +    const: 3
>> +    description: |
>> +      Use the same format as specified by GIC in arm,gic.yaml.
>
>That's odd. You have the same SPI and PPI stuff?
>

No, but I just want to keep the format same as arm,gic, and let the
driver bypass 1st and 3rd cell to the parent GIC.
The mst-intc driver translate the interrupt number in 2nd cell to the
interrupt on the parent GIC.

>> +
>> +  reg:
>> +    description: |
>> +      Physical base address of the mstar interrupt controller
>> +      registers and length of memory mapped region.
>
>Drop this. That's every 'reg' property.
>
>> +    minItems: 1
>
>maxItems is more logical.
>
>> +
>> +  mstar,irqs-map-range:
>> +    description: |
>> +      The range of parent interrupt controller's interrupt lines
>> +      that are hardwired to mstar interrupt controller.
>
>Is this <start size> or <start end>?
>

<start end>.
I will add this in the description.

>Really, this should just use 'interrupts' even though that's a bit 
>verbose. Or be implied by the compatible string. What's the maximum 
>number of parent interrupts?
>
>In any case, we really need to stop having vendor specific properties 
>for this.

We use 64 interrupts per interrupt controller.
As you mentions, if we use the standard property 'interrupts',
then we need to put 64 interrupt property in the interrupt
controller node, and it will be hard to understand.
So I suppose we need an vendor specific property here.

>
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    items:
>> +      minItems: 2
>> +      maxItems: 2
>> +
>> +  mstar,intc-no-eoi:
>> +    description: |
>
>Don't need '|' if there's no formatting.
>
>> +      Mark this controller has no End Of Interrupt(EOI) implementation.
>> +      This is a empty, boolean property.
>
>You can drop this line. The schema says this.
>
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - mstar,irqs-map-range
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    mst_intc0: interrupt-controller@1f2032d0 {
>> +      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
>> +      interrupt-controller;
>> +      #interrupt-cells = <3>;
>> +      #address-cells = <1>;
>> +      #size-cells = <1>;
>> +      interrupt-parent = <&gic>;
>> +      reg = <0x1f2032d0 0x30>;
>> +      mstar,irqs-map-range = <0 63>;
>
>Is 0 a PPI or SPI? This property is making some assumption and wouldn't 
>be able to support both types or another parent interrupt controller.
>

0 is the interrupt number of the parent interrupt controller.
There's no SPI and PPI stuff in mst-intc, it will bypass the
1st cell to parent controller.

e.g. The device node as following will point to GIC SPI 31
("value in 2nd cell" + "start in irqs-map-range").
	usb: {
		interrupt-parent = <&mtk_intc0>;
		interrupts = <0x0 31 0x4>;
		...
	};

>> +    };
>> +...
>> -- 
>> 2.18.0
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
new file mode 100644
index 000000000000..6e383315e529
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
@@ -0,0 +1,82 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/mstar,mst-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar Interrupt Controller
+
+maintainers:
+  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+
+description: |+
+  MStar, SigmaStar and Mediatek DTV SoCs contain multiple legacy
+  interrupt controllers that routes interrupts to the GIC.
+
+  The HW block exposes a number of interrupt controllers, each
+  can support up to 64 interrupts.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: mstar,mst-intc
+      - enum:
+          - mediatek,mt58xx-intc
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 0, 1, 2 ]
+
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+  "#interrupt-cells":
+    const: 3
+    description: |
+      Use the same format as specified by GIC in arm,gic.yaml.
+
+  reg:
+    description: |
+      Physical base address of the mstar interrupt controller
+      registers and length of memory mapped region.
+    minItems: 1
+
+  mstar,irqs-map-range:
+    description: |
+      The range of parent interrupt controller's interrupt lines
+      that are hardwired to mstar interrupt controller.
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      minItems: 2
+      maxItems: 2
+
+  mstar,intc-no-eoi:
+    description: |
+      Mark this controller has no End Of Interrupt(EOI) implementation.
+      This is a empty, boolean property.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - mstar,irqs-map-range
+
+additionalProperties: false
+
+examples:
+  - |
+    mst_intc0: interrupt-controller@1f2032d0 {
+      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
+      interrupt-controller;
+      #interrupt-cells = <3>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      interrupt-parent = <&gic>;
+      reg = <0x1f2032d0 0x30>;
+      mstar,irqs-map-range = <0 63>;
+    };
+...