diff mbox series

[v2,1/3] dt-bindings: PCI: ti,j721e: Add bindings to specify legacy interrupts

Message ID 20210804132912.30685-2-kishon@ti.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: Add legacy interrupt support in pci-j721e | expand

Commit Message

Kishon Vijay Abraham I Aug. 4, 2021, 1:29 p.m. UTC
Add bindings to specify interrupt controller for legacy interrupts.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Marc Zyngier Aug. 4, 2021, 3:05 p.m. UTC | #1
On Wed, 04 Aug 2021 14:29:10 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add bindings to specify interrupt controller for legacy interrupts.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> index cc900202df29..f461d7b4c0cc 100644
> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> @@ -74,6 +74,11 @@ properties:
>  
>    msi-map: true
>  
> +patternProperties:
> +  "interrupt-controller":
> +    type: object
> +    description: interrupt controller to handle legacy interrupts.
> +
>  required:
>    - compatible
>    - reg
> @@ -97,6 +102,8 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>      #include <dt-bindings/gpio/gpio.h>
>  
> @@ -131,5 +138,13 @@ examples:
>              ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
>                       <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
> +
> +
> +            pcie0_intc: interrupt-controller {
> +                    interrupt-controller;
> +                    #interrupt-cells = <1>;
> +                    interrupt-parent = <&gic500>;
> +                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;

Are you sure about the edge signalling? How is the interrupt
retriggered when the input is still high, which could well be the case
for shared INTx?

Thanks,

	M.
Kishon Vijay Abraham I Aug. 9, 2021, 4:38 a.m. UTC | #2
Hi Marc,

On 04/08/21 8:35 pm, Marc Zyngier wrote:
> On Wed, 04 Aug 2021 14:29:10 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Add bindings to specify interrupt controller for legacy interrupts.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> index cc900202df29..f461d7b4c0cc 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> @@ -74,6 +74,11 @@ properties:
>>  
>>    msi-map: true
>>  
>> +patternProperties:
>> +  "interrupt-controller":
>> +    type: object
>> +    description: interrupt controller to handle legacy interrupts.
>> +
>>  required:
>>    - compatible
>>    - reg
>> @@ -97,6 +102,8 @@ unevaluatedProperties: false
>>  
>>  examples:
>>    - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>      #include <dt-bindings/gpio/gpio.h>
>>  
>> @@ -131,5 +138,13 @@ examples:
>>              ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
>>                       <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
>>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
>> +
>> +
>> +            pcie0_intc: interrupt-controller {
>> +                    interrupt-controller;
>> +                    #interrupt-cells = <1>;
>> +                    interrupt-parent = <&gic500>;
>> +                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
> 
> Are you sure about the edge signalling? How is the interrupt
> retriggered when the input is still high, which could well be the case
> for shared INTx?

There is a EOI register which is used for re-triggering the interrupt.
That functionality is broken in J721E but is fixed in J7200 (the
following two patches in the series deals with that).

Thanks,
Kishon
Rob Herring Aug. 13, 2021, 5:17 p.m. UTC | #3
On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
> Add bindings to specify interrupt controller for legacy interrupts.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> index cc900202df29..f461d7b4c0cc 100644
> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> @@ -74,6 +74,11 @@ properties:
>  
>    msi-map: true
>  
> +patternProperties:
> +  "interrupt-controller":

Not a pattern unless you meant for foo-interrupt-controller-bar to be 
valid.

Anything is allowed in the node?

> +    type: object
> +    description: interrupt controller to handle legacy interrupts.
> +
>  required:
>    - compatible
>    - reg
> @@ -97,6 +102,8 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>      #include <dt-bindings/gpio/gpio.h>
>  
> @@ -131,5 +138,13 @@ examples:
>              ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
>                       <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
> +
> +
> +            pcie0_intc: interrupt-controller {
> +                    interrupt-controller;
> +                    #interrupt-cells = <1>;
> +                    interrupt-parent = <&gic500>;
> +                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
> +            };
>          };
>      };
> -- 
> 2.17.1
> 
>
Kishon Vijay Abraham I Aug. 18, 2021, 1:58 p.m. UTC | #4
Hi Rob,

On 13/08/21 10:47 pm, Rob Herring wrote:
> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
>> Add bindings to specify interrupt controller for legacy interrupts.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> index cc900202df29..f461d7b4c0cc 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> @@ -74,6 +74,11 @@ properties:
>>  
>>    msi-map: true
>>  
>> +patternProperties:
>> +  "interrupt-controller":
> 
> Not a pattern unless you meant for foo-interrupt-controller-bar to be 
> valid.
> 
> Anything is allowed in the node?

It's same as whatever is defined in schemas/interrupt-controller.yaml,
just that it should be a subnode of pcie@. Should I add whatever is
present in schemas/interrupt-controller.yaml here?

Thanks
Kishon
> 
>> +    type: object
>> +    description: interrupt controller to handle legacy interrupts.
>> +
>>  required:
>>    - compatible
>>    - reg
>> @@ -97,6 +102,8 @@ unevaluatedProperties: false
>>  
>>  examples:
>>    - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>      #include <dt-bindings/gpio/gpio.h>
>>  
>> @@ -131,5 +138,13 @@ examples:
>>              ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
>>                       <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
>>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
>> +
>> +
>> +            pcie0_intc: interrupt-controller {
>> +                    interrupt-controller;
>> +                    #interrupt-cells = <1>;
>> +                    interrupt-parent = <&gic500>;
>> +                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
>> +            };
>>          };
>>      };
>> -- 
>> 2.17.1
>>
>>
Kishon Vijay Abraham I Sept. 23, 2021, 4:33 a.m. UTC | #5
Hi Rob,

On 18/08/21 7:28 pm, Kishon Vijay Abraham I wrote:
> Hi Rob,
> 
> On 13/08/21 10:47 pm, Rob Herring wrote:
>> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
>>> Add bindings to specify interrupt controller for legacy interrupts.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> index cc900202df29..f461d7b4c0cc 100644
>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>> @@ -74,6 +74,11 @@ properties:
>>>  
>>>    msi-map: true
>>>  
>>> +patternProperties:
>>> +  "interrupt-controller":
>>
>> Not a pattern unless you meant for foo-interrupt-controller-bar to be 
>> valid.
>>
>> Anything is allowed in the node?
> 
> It's same as whatever is defined in schemas/interrupt-controller.yaml,
> just that it should be a subnode of pcie@. Should I add whatever is
> present in schemas/interrupt-controller.yaml here?

Can you suggest how to include this?

Thanks,
Kishon

> 
> Thanks
> Kishon
>>
>>> +    type: object
>>> +    description: interrupt controller to handle legacy interrupts.
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -97,6 +102,8 @@ unevaluatedProperties: false
>>>  
>>>  examples:
>>>    - |
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>      #include <dt-bindings/soc/ti,sci_pm_domain.h>
>>>      #include <dt-bindings/gpio/gpio.h>
>>>  
>>> @@ -131,5 +138,13 @@ examples:
>>>              ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
>>>                       <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
>>>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
>>> +
>>> +
>>> +            pcie0_intc: interrupt-controller {
>>> +                    interrupt-controller;
>>> +                    #interrupt-cells = <1>;
>>> +                    interrupt-parent = <&gic500>;
>>> +                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
>>> +            };
>>>          };
>>>      };
>>> -- 
>>> 2.17.1
>>>
>>>
Rob Herring Sept. 23, 2021, 3:44 p.m. UTC | #6
On Wed, Sep 22, 2021 at 11:33 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Rob,
>
> On 18/08/21 7:28 pm, Kishon Vijay Abraham I wrote:
> > Hi Rob,
> >
> > On 13/08/21 10:47 pm, Rob Herring wrote:
> >> On Wed, Aug 04, 2021 at 06:59:10PM +0530, Kishon Vijay Abraham I wrote:
> >>> Add bindings to specify interrupt controller for legacy interrupts.
> >>>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> ---
> >>>  .../bindings/pci/ti,j721e-pci-host.yaml           | 15 +++++++++++++++
> >>>  1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> >>> index cc900202df29..f461d7b4c0cc 100644
> >>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> >>> @@ -74,6 +74,11 @@ properties:
> >>>
> >>>    msi-map: true
> >>>
> >>> +patternProperties:
> >>> +  "interrupt-controller":
> >>
> >> Not a pattern unless you meant for foo-interrupt-controller-bar to be
> >> valid.
> >>
> >> Anything is allowed in the node?
> >
> > It's same as whatever is defined in schemas/interrupt-controller.yaml,
> > just that it should be a subnode of pcie@. Should I add whatever is
> > present in schemas/interrupt-controller.yaml here?
>
> Can you suggest how to include this?

You don't. List the properties you are using. You need to define the
#interrupt-cells value for example. I'm sure there's already examples
in the tree doing this same child node.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index cc900202df29..f461d7b4c0cc 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -74,6 +74,11 @@  properties:
 
   msi-map: true
 
+patternProperties:
+  "interrupt-controller":
+    type: object
+    description: interrupt controller to handle legacy interrupts.
+
 required:
   - compatible
   - reg
@@ -97,6 +102,8 @@  unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/soc/ti,sci_pm_domain.h>
     #include <dt-bindings/gpio/gpio.h>
 
@@ -131,5 +138,13 @@  examples:
             ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
                      <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+
+
+            pcie0_intc: interrupt-controller {
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+                    interrupt-parent = <&gic500>;
+                    interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
+            };
         };
     };