linux-kernel - Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MStar interrupt controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Aug 2020 15:50:38 +0800
From:   Mark-PK Tsai <mark-pk.tsai@...iatek.com>
To:     <robh@...nel.org>, Mark-PK Tsai <mark-pk.tsai@...iatek.com>
CC:     <alix.wu@...iatek.com>, <daniel@...f.com>,
        <devicetree@...r.kernel.org>, <jason@...edaemon.net>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <maz@...nel.org>, <tglx@...utronix.de>, <yj.chiang@...iatek.com>
Subject: Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MStar interrupt controller

From: Rob Herring <robh@...nel.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@...032d0 {
>> +      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