| Message ID | 1527631130-20045-2-git-send-email-ray.jui@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PAXB INTx support with proper model | expand |
+Arnd On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: > Update the iProc PCIe binding document for better modeling of the legacy > interrupt (INTx) support > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > --- > .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > index b8e48b4..7ea24dc 100644 > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > @@ -13,9 +13,6 @@ controller, used in Stingray > PAXB-based root complex is used for external endpoint devices. PAXC-based > root complex is connected to emulated endpoint devices internal to the ASIC > - reg: base address and length of the PCIe controller I/O register space > -- #interrupt-cells: set to <1> > -- interrupt-map-mask and interrupt-map, standard PCI properties to define the > - mapping of the PCIe interface to interrupt numbers > - linux,pci-domain: PCI domain ID. Should be unique for each host controller > - bus-range: PCI bus numbers covered > - #address-cells: set to <3> > @@ -41,6 +38,16 @@ Required: > - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal > address used by the iProc PCIe core (not the PCIe address) > > +Legacy interrupt (INTx) support (optional): > + > +Note INTx is for PAXB only. > + > +- interrupt-controller: claims itself as an interrupt controller for INTx > +- #interrupt-cells: set to <1> > +- interrupt-map-mask and interrupt-map, standard PCI properties to define > +the mapping of the PCIe interface to interrupt numbers > +- interrupts: interrupt line wired to the generic GIC for INTx support > + > MSI support (optional): > > For older platforms without MSI integrated in the GIC, iProc PCIe core provides > @@ -77,9 +84,14 @@ Example: > compatible = "brcm,iproc-pcie"; > reg = <0x18012000 0x1000>; > > + interrupt-controller; > #interrupt-cells = <1>; > - interrupt-map-mask = <0 0 0 0>; > - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; > + interrupt-map-mask = <0 0 0 7>; > + interrupt-map = <0 0 0 1 &pcie0 1>, Are you sure this works? The irq parsing code will ignore interrupt-map if interrupt-controller is found. In other words, you should have one or the other, but not both. Maybe it happens to work because "pcie0" is this node and your irq numbers are the same. Arnd, any thoughts on this? > + <0 0 0 2 &pcie0 2>, > + <0 0 0 3 &pcie0 3>, > + <0 0 0 4 &pcie0 4>; > + interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>; > > linux,pci-domain = <0>; > > @@ -115,9 +127,14 @@ Example: > compatible = "brcm,iproc-pcie"; > reg = <0x18013000 0x1000>; > > + interrupt-controller; > #interrupt-cells = <1>; > - interrupt-map-mask = <0 0 0 0>; > - interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>; > + interrupt-map-mask = <0 0 0 7>; > + interrupt-map = <0 0 0 1 &pcie1 1>, > + <0 0 0 2 &pcie1 2>, > + <0 0 0 3 &pcie1 3>, > + <0 0 0 4 &pcie1 4>; > + interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>; > > linux,pci-domain = <1>; > > -- > 2.1.4 >
Hi Rob/Arnd, On 6/4/2018 7:17 AM, Rob Herring wrote: > +Arnd > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: >> Update the iProc PCIe binding document for better modeling of the legacy >> interrupt (INTx) support >> >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> --- >> .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- >> 1 file changed, 24 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >> index b8e48b4..7ea24dc 100644 >> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >> @@ -13,9 +13,6 @@ controller, used in Stingray >> PAXB-based root complex is used for external endpoint devices. PAXC-based >> root complex is connected to emulated endpoint devices internal to the ASIC >> - reg: base address and length of the PCIe controller I/O register space >> -- #interrupt-cells: set to <1> >> -- interrupt-map-mask and interrupt-map, standard PCI properties to define the >> - mapping of the PCIe interface to interrupt numbers >> - linux,pci-domain: PCI domain ID. Should be unique for each host controller >> - bus-range: PCI bus numbers covered >> - #address-cells: set to <3> >> @@ -41,6 +38,16 @@ Required: >> - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal >> address used by the iProc PCIe core (not the PCIe address) >> >> +Legacy interrupt (INTx) support (optional): >> + >> +Note INTx is for PAXB only. >> + >> +- interrupt-controller: claims itself as an interrupt controller for INTx >> +- #interrupt-cells: set to <1> >> +- interrupt-map-mask and interrupt-map, standard PCI properties to define >> +the mapping of the PCIe interface to interrupt numbers >> +- interrupts: interrupt line wired to the generic GIC for INTx support >> + >> MSI support (optional): >> >> For older platforms without MSI integrated in the GIC, iProc PCIe core provides >> @@ -77,9 +84,14 @@ Example: >> compatible = "brcm,iproc-pcie"; >> reg = <0x18012000 0x1000>; >> >> + interrupt-controller; >> #interrupt-cells = <1>; >> - interrupt-map-mask = <0 0 0 0>; >> - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; >> + interrupt-map-mask = <0 0 0 7>; >> + interrupt-map = <0 0 0 1 &pcie0 1>, > > Are you sure this works? The irq parsing code will ignore > interrupt-map if interrupt-controller is found. In other words, you > should have one or the other, but not both. Yes, it does work. I tested this by using an Intel E1000E PCIe NIC card installed in our system and have it fall back to INTx. > > Maybe it happens to work because "pcie0" is this node and your irq > numbers are the same. Perhaps it works because we are claiming "pcie0" as an interrupt controller by itself and the INTx is modeled under that. > > Arnd, any thoughts on this? > Please let me know if the above model makes sense or not. Thanks, Ray >> + <0 0 0 2 &pcie0 2>, >> + <0 0 0 3 &pcie0 3>, >> + <0 0 0 4 &pcie0 4>; >> + interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>; >> >> linux,pci-domain = <0>; >> >> @@ -115,9 +127,14 @@ Example: >> compatible = "brcm,iproc-pcie"; >> reg = <0x18013000 0x1000>; >> >> + interrupt-controller; >> #interrupt-cells = <1>; >> - interrupt-map-mask = <0 0 0 0>; >> - interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>; >> + interrupt-map-mask = <0 0 0 7>; >> + interrupt-map = <0 0 0 1 &pcie1 1>, >> + <0 0 0 2 &pcie1 2>, >> + <0 0 0 3 &pcie1 3>, >> + <0 0 0 4 &pcie1 4>; >> + interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>; >> >> linux,pci-domain = <1>; >> >> -- >> 2.1.4 >>
On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote: > +Arnd > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: > > Update the iProc PCIe binding document for better modeling of the legacy > > interrupt (INTx) support > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > > --- > > .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > index b8e48b4..7ea24dc 100644 > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > @@ -13,9 +13,6 @@ controller, used in Stingray > > PAXB-based root complex is used for external endpoint devices. PAXC-based > > root complex is connected to emulated endpoint devices internal to the ASIC > > - reg: base address and length of the PCIe controller I/O register space > > -- #interrupt-cells: set to <1> > > -- interrupt-map-mask and interrupt-map, standard PCI properties to define the > > - mapping of the PCIe interface to interrupt numbers > > - linux,pci-domain: PCI domain ID. Should be unique for each host controller > > - bus-range: PCI bus numbers covered > > - #address-cells: set to <3> > > @@ -41,6 +38,16 @@ Required: > > - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal > > address used by the iProc PCIe core (not the PCIe address) > > > > +Legacy interrupt (INTx) support (optional): > > + > > +Note INTx is for PAXB only. > > + > > +- interrupt-controller: claims itself as an interrupt controller for INTx > > +- #interrupt-cells: set to <1> > > +- interrupt-map-mask and interrupt-map, standard PCI properties to define > > +the mapping of the PCIe interface to interrupt numbers > > +- interrupts: interrupt line wired to the generic GIC for INTx support > > + > > MSI support (optional): > > > > For older platforms without MSI integrated in the GIC, iProc PCIe core provides > > @@ -77,9 +84,14 @@ Example: > > compatible = "brcm,iproc-pcie"; > > reg = <0x18012000 0x1000>; > > > > + interrupt-controller; > > #interrupt-cells = <1>; > > - interrupt-map-mask = <0 0 0 0>; > > - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; > > + interrupt-map-mask = <0 0 0 7>; > > + interrupt-map = <0 0 0 1 &pcie0 1>, > > Are you sure this works? The irq parsing code will ignore > interrupt-map if interrupt-controller is found. In other words, you > should have one or the other, but not both. > > Maybe it happens to work because "pcie0" is this node and your irq > numbers are the same. > > Arnd, any thoughts on this? To start with, I think the destination IRQ number is wrong, what the mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB, #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo) interrupt controller. I really want to clean this up since currently there are different DT bindings defining this in different ways which resulted in non-consistent kernel code. AFAICS, the Aardvark PCIe controller bindings define the mapping as I expect: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4 but I would like to get Rob and Arnd viewpoint on this so that we can close this topic once for all. Cheers, Lorenzo > > > + <0 0 0 2 &pcie0 2>, > > + <0 0 0 3 &pcie0 3>, > > + <0 0 0 4 &pcie0 4>; > > + interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>; > > > > linux,pci-domain = <0>; > > > > @@ -115,9 +127,14 @@ Example: > > compatible = "brcm,iproc-pcie"; > > reg = <0x18013000 0x1000>; > > > > + interrupt-controller; > > #interrupt-cells = <1>; > > - interrupt-map-mask = <0 0 0 0>; > > - interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>; > > + interrupt-map-mask = <0 0 0 7>; > > + interrupt-map = <0 0 0 1 &pcie1 1>, > > + <0 0 0 2 &pcie1 2>, > > + <0 0 0 3 &pcie1 3>, > > + <0 0 0 4 &pcie1 4>; > > + interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>; > > > > linux,pci-domain = <1>; > > > > -- > > 2.1.4 > >
On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote: > > +Arnd > > > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: > > > Update the iProc PCIe binding document for better modeling of the legacy > > > interrupt (INTx) support > > > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > > > --- > > > .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > index b8e48b4..7ea24dc 100644 > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > @@ -13,9 +13,6 @@ controller, used in Stingray > > > PAXB-based root complex is used for external endpoint devices. PAXC-based > > > root complex is connected to emulated endpoint devices internal to the ASIC > > > - reg: base address and length of the PCIe controller I/O register space > > > -- #interrupt-cells: set to <1> > > > -- interrupt-map-mask and interrupt-map, standard PCI properties to define the > > > - mapping of the PCIe interface to interrupt numbers > > > - linux,pci-domain: PCI domain ID. Should be unique for each host controller > > > - bus-range: PCI bus numbers covered > > > - #address-cells: set to <3> > > > @@ -41,6 +38,16 @@ Required: > > > - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal > > > address used by the iProc PCIe core (not the PCIe address) > > > > > > +Legacy interrupt (INTx) support (optional): > > > + > > > +Note INTx is for PAXB only. > > > + > > > +- interrupt-controller: claims itself as an interrupt controller for INTx > > > +- #interrupt-cells: set to <1> > > > +- interrupt-map-mask and interrupt-map, standard PCI properties to define > > > +the mapping of the PCIe interface to interrupt numbers > > > +- interrupts: interrupt line wired to the generic GIC for INTx support > > > + > > > MSI support (optional): > > > > > > For older platforms without MSI integrated in the GIC, iProc PCIe core provides > > > @@ -77,9 +84,14 @@ Example: > > > compatible = "brcm,iproc-pcie"; > > > reg = <0x18012000 0x1000>; > > > > > > + interrupt-controller; > > > #interrupt-cells = <1>; > > > - interrupt-map-mask = <0 0 0 0>; > > > - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; > > > + interrupt-map-mask = <0 0 0 7>; > > > + interrupt-map = <0 0 0 1 &pcie0 1>, > > > > Are you sure this works? The irq parsing code will ignore > > interrupt-map if interrupt-controller is found. In other words, you > > should have one or the other, but not both. > > > > Maybe it happens to work because "pcie0" is this node and your irq > > numbers are the same. > > > > Arnd, any thoughts on this? > > To start with, I think the destination IRQ number is wrong, what the > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB, > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo) > interrupt controller. > > I really want to clean this up since currently there are different > DT bindings defining this in different ways which resulted in > non-consistent kernel code. > > AFAICS, the Aardvark PCIe controller bindings define the mapping > as I expect: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4 > > but I would like to get Rob and Arnd viewpoint on this so that > we can close this topic once for all. It seems ambiguous at best, as Rob suggested it may only work by accident. Since there is only one upstream interrupt, could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as the destination for any IntX? Arnd
On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote: > > > +Arnd > > > > > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: > > > > Update the iProc PCIe binding document for better modeling of the legacy > > > > interrupt (INTx) support > > > > > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > > > > --- > > > > .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > index b8e48b4..7ea24dc 100644 > > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > @@ -13,9 +13,6 @@ controller, used in Stingray > > > > PAXB-based root complex is used for external endpoint devices. PAXC-based > > > > root complex is connected to emulated endpoint devices internal to the ASIC > > > > - reg: base address and length of the PCIe controller I/O register space > > > > -- #interrupt-cells: set to <1> > > > > -- interrupt-map-mask and interrupt-map, standard PCI properties to define the > > > > - mapping of the PCIe interface to interrupt numbers > > > > - linux,pci-domain: PCI domain ID. Should be unique for each host controller > > > > - bus-range: PCI bus numbers covered > > > > - #address-cells: set to <3> > > > > @@ -41,6 +38,16 @@ Required: > > > > - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal > > > > address used by the iProc PCIe core (not the PCIe address) > > > > > > > > +Legacy interrupt (INTx) support (optional): > > > > + > > > > +Note INTx is for PAXB only. > > > > + > > > > +- interrupt-controller: claims itself as an interrupt controller for INTx > > > > +- #interrupt-cells: set to <1> > > > > +- interrupt-map-mask and interrupt-map, standard PCI properties to define > > > > +the mapping of the PCIe interface to interrupt numbers > > > > +- interrupts: interrupt line wired to the generic GIC for INTx support > > > > + > > > > MSI support (optional): > > > > > > > > For older platforms without MSI integrated in the GIC, iProc PCIe core provides > > > > @@ -77,9 +84,14 @@ Example: > > > > compatible = "brcm,iproc-pcie"; > > > > reg = <0x18012000 0x1000>; > > > > > > > > + interrupt-controller; > > > > #interrupt-cells = <1>; > > > > - interrupt-map-mask = <0 0 0 0>; > > > > - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; > > > > + interrupt-map-mask = <0 0 0 7>; > > > > + interrupt-map = <0 0 0 1 &pcie0 1>, > > > > > > Are you sure this works? The irq parsing code will ignore > > > interrupt-map if interrupt-controller is found. In other words, you > > > should have one or the other, but not both. > > > > > > Maybe it happens to work because "pcie0" is this node and your irq > > > numbers are the same. > > > > > > Arnd, any thoughts on this? > > > > To start with, I think the destination IRQ number is wrong, what the > > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB, > > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo) > > interrupt controller. > > > > I really want to clean this up since currently there are different > > DT bindings defining this in different ways which resulted in > > non-consistent kernel code. > > > > AFAICS, the Aardvark PCIe controller bindings define the mapping > > as I expect: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4 > > > > but I would like to get Rob and Arnd viewpoint on this so that > > we can close this topic once for all. > > It seems ambiguous at best, as Rob suggested it may only > work by accident. Since there is only one upstream interrupt, > could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as > the destination for any IntX? I think that would not be correct from an HW description standpoint since there is some logic in the host bridge that behaves as an interrupt controller (eg registers to ack/mask IRQs). AFAICS the aardvark (it is an example) bindings below should be correct, with an interrupt controller node within the PCI host bridge: pcie0: pcie@d0070000 { compatible = "marvell,armada-3700-pcie"; device_type = "pci"; reg = <0 0xd0070000 0 0x20000>; #address-cells = <3>; #size-cells = <2>; bus-range = <0x00 0xff>; interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; #interrupt-cells = <1>; msi-controller; msi-parent = <&pcie0>; ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ interrupt-map-mask = <0 0 0 7>; interrupt-map = <0 0 0 1 &pcie_intc 0>, <0 0 0 2 &pcie_intc 1>, <0 0 0 3 &pcie_intc 2>, <0 0 0 4 &pcie_intc 3>; pcie_intc: interrupt-controller { interrupt-controller; #interrupt-cells = <1>; }; }; Thoughts ? Lorenzo
On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote: > > On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote: > > > > +Arnd > > > > > > > > On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: > > > > > Update the iProc PCIe binding document for better modeling of the legacy > > > > > interrupt (INTx) support > > > > > > > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > > > > > --- > > > > > .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- > > > > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > > index b8e48b4..7ea24dc 100644 > > > > > --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > > +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt > > > > > @@ -13,9 +13,6 @@ controller, used in Stingray > > > > > PAXB-based root complex is used for external endpoint devices. PAXC-based > > > > > root complex is connected to emulated endpoint devices internal to the ASIC > > > > > - reg: base address and length of the PCIe controller I/O register space > > > > > -- #interrupt-cells: set to <1> > > > > > -- interrupt-map-mask and interrupt-map, standard PCI properties to define the > > > > > - mapping of the PCIe interface to interrupt numbers > > > > > - linux,pci-domain: PCI domain ID. Should be unique for each host controller > > > > > - bus-range: PCI bus numbers covered > > > > > - #address-cells: set to <3> > > > > > @@ -41,6 +38,16 @@ Required: > > > > > - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal > > > > > address used by the iProc PCIe core (not the PCIe address) > > > > > > > > > > +Legacy interrupt (INTx) support (optional): > > > > > + > > > > > +Note INTx is for PAXB only. > > > > > + > > > > > +- interrupt-controller: claims itself as an interrupt controller for INTx > > > > > +- #interrupt-cells: set to <1> > > > > > +- interrupt-map-mask and interrupt-map, standard PCI properties to define > > > > > +the mapping of the PCIe interface to interrupt numbers > > > > > +- interrupts: interrupt line wired to the generic GIC for INTx support > > > > > + > > > > > MSI support (optional): > > > > > > > > > > For older platforms without MSI integrated in the GIC, iProc PCIe core provides > > > > > @@ -77,9 +84,14 @@ Example: > > > > > compatible = "brcm,iproc-pcie"; > > > > > reg = <0x18012000 0x1000>; > > > > > > > > > > + interrupt-controller; > > > > > #interrupt-cells = <1>; > > > > > - interrupt-map-mask = <0 0 0 0>; > > > > > - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; > > > > > + interrupt-map-mask = <0 0 0 7>; > > > > > + interrupt-map = <0 0 0 1 &pcie0 1>, > > > > > > > > Are you sure this works? The irq parsing code will ignore > > > > interrupt-map if interrupt-controller is found. In other words, you > > > > should have one or the other, but not both. > > > > > > > > Maybe it happens to work because "pcie0" is this node and your irq > > > > numbers are the same. > > > > > > > > Arnd, any thoughts on this? > > > > > > To start with, I think the destination IRQ number is wrong, what the > > > mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB, > > > #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo) > > > interrupt controller. > > > > > > I really want to clean this up since currently there are different > > > DT bindings defining this in different ways which resulted in > > > non-consistent kernel code. > > > > > > AFAICS, the Aardvark PCIe controller bindings define the mapping > > > as I expect: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4 > > > > > > but I would like to get Rob and Arnd viewpoint on this so that > > > we can close this topic once for all. > > > > It seems ambiguous at best, as Rob suggested it may only > > work by accident. Since there is only one upstream interrupt, > > could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as > > the destination for any IntX? > > I think that would not be correct from an HW description standpoint > since there is some logic in the host bridge that behaves as an > interrupt controller (eg registers to ack/mask IRQs). > > AFAICS the aardvark (it is an example) bindings below should be correct, > with an interrupt controller node within the PCI host bridge: > > pcie0: pcie@d0070000 { > compatible = "marvell,armada-3700-pcie"; > device_type = "pci"; > reg = <0 0xd0070000 0 0x20000>; > #address-cells = <3>; > #size-cells = <2>; > bus-range = <0x00 0xff>; > interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; > #interrupt-cells = <1>; > msi-controller; > msi-parent = <&pcie0>; > ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ > 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ > interrupt-map-mask = <0 0 0 7>; > interrupt-map = <0 0 0 1 &pcie_intc 0>, > <0 0 0 2 &pcie_intc 1>, > <0 0 0 3 &pcie_intc 2>, > <0 0 0 4 &pcie_intc 3>; > pcie_intc: interrupt-controller { > interrupt-controller; > #interrupt-cells = <1>; > }; > }; > > Thoughts ? Yes, I think that's better. We probably still need to move the interrupts, msi-controller, msi-parent and interrupt-parent properties into the child node. Arnd
On 9/25/2018 3:55 AM, Arnd Bergmann wrote: > On Tue, Sep 25, 2018 at 12:49 PM Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> >> On Mon, Sep 24, 2018 at 10:53:13PM +0200, Arnd Bergmann wrote: >>> On Tue, Sep 18, 2018 at 3:42 PM Lorenzo Pieralisi >>> <lorenzo.pieralisi@arm.com> wrote: >>>> >>>> On Mon, Jun 04, 2018 at 09:17:49AM -0500, Rob Herring wrote: >>>>> +Arnd >>>>> >>>>> On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote: >>>>>> Update the iProc PCIe binding document for better modeling of the legacy >>>>>> interrupt (INTx) support >>>>>> >>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>> --- >>>>>> .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- >>>>>> 1 file changed, 24 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >>>>>> index b8e48b4..7ea24dc 100644 >>>>>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >>>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt >>>>>> @@ -13,9 +13,6 @@ controller, used in Stingray >>>>>> PAXB-based root complex is used for external endpoint devices. PAXC-based >>>>>> root complex is connected to emulated endpoint devices internal to the ASIC >>>>>> - reg: base address and length of the PCIe controller I/O register space >>>>>> -- #interrupt-cells: set to <1> >>>>>> -- interrupt-map-mask and interrupt-map, standard PCI properties to define the >>>>>> - mapping of the PCIe interface to interrupt numbers >>>>>> - linux,pci-domain: PCI domain ID. Should be unique for each host controller >>>>>> - bus-range: PCI bus numbers covered >>>>>> - #address-cells: set to <3> >>>>>> @@ -41,6 +38,16 @@ Required: >>>>>> - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal >>>>>> address used by the iProc PCIe core (not the PCIe address) >>>>>> >>>>>> +Legacy interrupt (INTx) support (optional): >>>>>> + >>>>>> +Note INTx is for PAXB only. >>>>>> + >>>>>> +- interrupt-controller: claims itself as an interrupt controller for INTx >>>>>> +- #interrupt-cells: set to <1> >>>>>> +- interrupt-map-mask and interrupt-map, standard PCI properties to define >>>>>> +the mapping of the PCIe interface to interrupt numbers >>>>>> +- interrupts: interrupt line wired to the generic GIC for INTx support >>>>>> + >>>>>> MSI support (optional): >>>>>> >>>>>> For older platforms without MSI integrated in the GIC, iProc PCIe core provides >>>>>> @@ -77,9 +84,14 @@ Example: >>>>>> compatible = "brcm,iproc-pcie"; >>>>>> reg = <0x18012000 0x1000>; >>>>>> >>>>>> + interrupt-controller; >>>>>> #interrupt-cells = <1>; >>>>>> - interrupt-map-mask = <0 0 0 0>; >>>>>> - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; >>>>>> + interrupt-map-mask = <0 0 0 7>; >>>>>> + interrupt-map = <0 0 0 1 &pcie0 1>, >>>>> >>>>> Are you sure this works? The irq parsing code will ignore >>>>> interrupt-map if interrupt-controller is found. In other words, you >>>>> should have one or the other, but not both. >>>>> >>>>> Maybe it happens to work because "pcie0" is this node and your irq >>>>> numbers are the same. >>>>> >>>>> Arnd, any thoughts on this? >>>> >>>> To start with, I think the destination IRQ number is wrong, what the >>>> mappings actually do is mapping the PCI interrupt line (ie #INTA, #INTB, >>>> #INTC, #INTD) to input {0,1,2,3} of the PCI host bridge (pseudo) >>>> interrupt controller. >>>> >>>> I really want to clean this up since currently there are different >>>> DT bindings defining this in different ways which resulted in >>>> non-consistent kernel code. >>>> >>>> AFAICS, the Aardvark PCIe controller bindings define the mapping >>>> as I expect: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/aardvark-pci.txt?h=v4.19-rc4 >>>> >>>> but I would like to get Rob and Arnd viewpoint on this so that >>>> we can close this topic once for all. >>> >>> It seems ambiguous at best, as Rob suggested it may only >>> work by accident. Since there is only one upstream interrupt, >>> could we simply list <GIC_SPI 100 IRQ_TYPE_NONE> as >>> the destination for any IntX? >> >> I think that would not be correct from an HW description standpoint >> since there is some logic in the host bridge that behaves as an >> interrupt controller (eg registers to ack/mask IRQs). >> >> AFAICS the aardvark (it is an example) bindings below should be correct, >> with an interrupt controller node within the PCI host bridge: >> >> pcie0: pcie@d0070000 { >> compatible = "marvell,armada-3700-pcie"; >> device_type = "pci"; >> reg = <0 0xd0070000 0 0x20000>; >> #address-cells = <3>; >> #size-cells = <2>; >> bus-range = <0x00 0xff>; >> interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; >> #interrupt-cells = <1>; >> msi-controller; >> msi-parent = <&pcie0>; >> ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ >> 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ >> interrupt-map-mask = <0 0 0 7>; >> interrupt-map = <0 0 0 1 &pcie_intc 0>, >> <0 0 0 2 &pcie_intc 1>, >> <0 0 0 3 &pcie_intc 2>, >> <0 0 0 4 &pcie_intc 3>; >> pcie_intc: interrupt-controller { >> interrupt-controller; >> #interrupt-cells = <1>; >> }; >> }; >> >> Thoughts ? > > Yes, I think that's better. We probably still need to move the > interrupts, msi-controller, msi-parent and interrupt-parent > properties into the child node. Okay thanks for all the feedback. In my case, I think I just to need create a dummy 'intc' subnode under the pcie node and declare it as a (dummy) interrupt controller). I'll make the change in my next revision. > > Arnd >
diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt index b8e48b4..7ea24dc 100644 --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt @@ -13,9 +13,6 @@ controller, used in Stingray PAXB-based root complex is used for external endpoint devices. PAXC-based root complex is connected to emulated endpoint devices internal to the ASIC - reg: base address and length of the PCIe controller I/O register space -- #interrupt-cells: set to <1> -- interrupt-map-mask and interrupt-map, standard PCI properties to define the - mapping of the PCIe interface to interrupt numbers - linux,pci-domain: PCI domain ID. Should be unique for each host controller - bus-range: PCI bus numbers covered - #address-cells: set to <3> @@ -41,6 +38,16 @@ Required: - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal address used by the iProc PCIe core (not the PCIe address) +Legacy interrupt (INTx) support (optional): + +Note INTx is for PAXB only. + +- interrupt-controller: claims itself as an interrupt controller for INTx +- #interrupt-cells: set to <1> +- interrupt-map-mask and interrupt-map, standard PCI properties to define +the mapping of the PCIe interface to interrupt numbers +- interrupts: interrupt line wired to the generic GIC for INTx support + MSI support (optional): For older platforms without MSI integrated in the GIC, iProc PCIe core provides @@ -77,9 +84,14 @@ Example: compatible = "brcm,iproc-pcie"; reg = <0x18012000 0x1000>; + interrupt-controller; #interrupt-cells = <1>; - interrupt-map-mask = <0 0 0 0>; - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>; + interrupt-map-mask = <0 0 0 7>; + interrupt-map = <0 0 0 1 &pcie0 1>, + <0 0 0 2 &pcie0 2>, + <0 0 0 3 &pcie0 3>, + <0 0 0 4 &pcie0 4>; + interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>; linux,pci-domain = <0>; @@ -115,9 +127,14 @@ Example: compatible = "brcm,iproc-pcie"; reg = <0x18013000 0x1000>; + interrupt-controller; #interrupt-cells = <1>; - interrupt-map-mask = <0 0 0 0>; - interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>; + interrupt-map-mask = <0 0 0 7>; + interrupt-map = <0 0 0 1 &pcie1 1>, + <0 0 0 2 &pcie1 2>, + <0 0 0 3 &pcie1 3>, + <0 0 0 4 &pcie1 4>; + interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>; linux,pci-domain = <1>;
Update the iProc PCIe binding document for better modeling of the legacy interrupt (INTx) support Signed-off-by: Ray Jui <ray.jui@broadcom.com> --- .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-)