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

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

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

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