| Message ID | 20240626104544.14233-2-svarbanov@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PCIe support for bcm2712 | expand |
On 26/06/2024 11:45, Stanimir Varbanov wrote: > Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. > > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > --- > .../brcm,bcm2712-msix.yaml | 74 +++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > new file mode 100644 > index 000000000000..ca610e4467d9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > @@ -0,0 +1,74 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom bcm2712 MSI-X Interrupt Peripheral support > + > +maintainers: > + - Stanimir Varbanov <svarbanov@suse.de> > + > +description: > > + This interrupt controller is used to provide intterupt vectors to the > + generic interrupt controller (GIC) on bcm2712. It will be used as > + external MSI-X controller for PCIe root complex. > + > +allOf: > + - $ref: /schemas/interrupt-controller/msi-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - "brcm,bcm2712-mip-intc" > + reg: > + maxItems: 1 > + description: > > + Specifies the base physical address and size of the registers > + > + interrupt-controller: true > + > + "#interrupt-cells": > + const: 2 > + > + msi-controller: true > + > + brcm,msi-base-spi: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The SGI number that MSIs start. > + > + brcm,msi-num-spis: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The number of SGIs for MSIs. > + > + brcm,msi-offset: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Shift the allocated MSIs up by N. > + > + brcm,msi-pci-addr: > + $ref: /schemas/types.yaml#/definitions/uint64 > + description: MSI-X message address. > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupt-controller > + - "#interrupt-cells" > + - msi-controller From the implementation of the driver, it looks like all properties are required, except for brcm,msi-offset which has a fallback to the value 0.
On 26/06/2024 11:45, Stanimir Varbanov wrote: > Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. > > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > --- > .../brcm,bcm2712-msix.yaml | 74 +++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > new file mode 100644 > index 000000000000..ca610e4467d9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > @@ -0,0 +1,74 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom bcm2712 MSI-X Interrupt Peripheral support > + > +maintainers: > + - Stanimir Varbanov <svarbanov@suse.de> > + > +description: > > + This interrupt controller is used to provide intterupt vectors to the > + generic interrupt controller (GIC) on bcm2712. It will be used as > + external MSI-X controller for PCIe root complex. > + > +allOf: > + - $ref: /schemas/interrupt-controller/msi-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - "brcm,bcm2712-mip-intc" > + reg: > + maxItems: 1 > + description: > > + Specifies the base physical address and size of the registers > + > + interrupt-controller: true > + > + "#interrupt-cells": > + const: 2 Should we have some sort of an interrupt-map or interrupt-map-mask property that defines the "linkage" between the inputs and the outputs? This controller does not really sit at the top-level of the interrupt tree as it feeds the ARM GIC, unfortunately this is not captured at all, and it seems to require ad-hoc properties to establish the mapping, that does not seem ideal.
On Wed, Jun 26, 2024 at 01:45:38PM +0300, Stanimir Varbanov wrote: > Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. > > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > --- > .../brcm,bcm2712-msix.yaml | 74 +++++++++++++++++++ > 1 file changed, 74 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > new file mode 100644 > index 000000000000..ca610e4467d9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml > @@ -0,0 +1,74 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom bcm2712 MSI-X Interrupt Peripheral support > + > +maintainers: > + - Stanimir Varbanov <svarbanov@suse.de> > + > +description: > > + This interrupt controller is used to provide intterupt vectors to the typo > + generic interrupt controller (GIC) on bcm2712. It will be used as > + external MSI-X controller for PCIe root complex. > + > +allOf: > + - $ref: /schemas/interrupt-controller/msi-controller.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - "brcm,bcm2712-mip-intc" Don't need quotes. Nor 'items'. And enum can be 'const' > + reg: > + maxItems: 1 > + description: > > + Specifies the base physical address and size of the registers drop. That's *every* reg property. > + > + interrupt-controller: true > + > + "#interrupt-cells": > + const: 2 > + > + msi-controller: true Add #msi-cells. The default is 0, but that's legacy. > + > + brcm,msi-base-spi: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The SGI number that MSIs start. > + > + brcm,msi-num-spis: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The number of SGIs for MSIs. > + > + brcm,msi-offset: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Shift the allocated MSIs up by N. If only we had a property that every MSI controller seems to need. Go check msi-controller.yaml... > + > + brcm,msi-pci-addr: > + $ref: /schemas/types.yaml#/definitions/uint64 > + description: MSI-X message address. Why don't other platforms need something like this? > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupt-controller > + - "#interrupt-cells" > + - msi-controller > + > +examples: > + - | > + msi-controller@130000 { > + compatible = "brcm,bcm2712-mip-intc"; > + reg = <0x00130000 0xc0>; > + msi-controller; > + interrupt-controller; > + #interrupt-cells = <2>; > + brcm,msi-base-spi = <128>; > + brcm,msi-num-spis = <64>; > + brcm,msi-offset = <0>; > + brcm,msi-pci-addr = <0xff 0xfffff000>; > + }; > -- > 2.43.0 >
Hi, Thank you for the review! >> +required: >> + - compatible >> + - reg >> + - interrupt-controller >> + - "#interrupt-cells" >> + - msi-controller > > From the implementation of the driver, it looks like all properties are > required, except for brcm,msi-offset which has a fallback to the value 0. Yes, correct. Will update in next revision. ~Stan
Hi, Thank you for the review! On 6/26/24 14:35, Florian Fainelli wrote: > > > On 26/06/2024 11:45, Stanimir Varbanov wrote: >> Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> <cut> >> +description: > >> + This interrupt controller is used to provide intterupt vectors to the >> + generic interrupt controller (GIC) on bcm2712. It will be used as >> + external MSI-X controller for PCIe root complex. >> + >> +allOf: >> + - $ref: /schemas/interrupt-controller/msi-controller.yaml# >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - "brcm,bcm2712-mip-intc" >> + reg: >> + maxItems: 1 >> + description: > >> + Specifies the base physical address and size of the registers >> + >> + interrupt-controller: true >> + >> + "#interrupt-cells": >> + const: 2 > > Should we have some sort of an interrupt-map or interrupt-map-mask > property that defines the "linkage" between the inputs and the outputs? > This controller does not really sit at the top-level of the interrupt > tree as it feeds the ARM GIC, unfortunately this is not captured at all, > and it seems to require ad-hoc properties to establish the mapping, that > does not seem ideal. Thank you for the suggestion. I will consider it. ~Stan
Hi Rob, Thank you for the comments! On 6/29/24 01:05, Rob Herring wrote: > On Wed, Jun 26, 2024 at 01:45:38PM +0300, Stanimir Varbanov wrote: >> Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> >> --- >> .../brcm,bcm2712-msix.yaml | 74 +++++++++++++++++++ >> 1 file changed, 74 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml >> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml >> new file mode 100644 >> index 000000000000..ca610e4467d9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml >> @@ -0,0 +1,74 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Broadcom bcm2712 MSI-X Interrupt Peripheral support >> + >> +maintainers: >> + - Stanimir Varbanov <svarbanov@suse.de> >> + >> +description: > >> + This interrupt controller is used to provide intterupt vectors to the > > typo Will fix it. > >> + generic interrupt controller (GIC) on bcm2712. It will be used as >> + external MSI-X controller for PCIe root complex. >> + >> +allOf: >> + - $ref: /schemas/interrupt-controller/msi-controller.yaml# >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - "brcm,bcm2712-mip-intc" > > Don't need quotes. Nor 'items'. And enum can be 'const' OK. > >> + reg: >> + maxItems: 1 >> + description: > >> + Specifies the base physical address and size of the registers > > drop. That's *every* reg property. OK. > >> + >> + interrupt-controller: true >> + >> + "#interrupt-cells": >> + const: 2 >> + >> + msi-controller: true > > Add #msi-cells. The default is 0, but that's legacy. OK. > >> + >> + brcm,msi-base-spi: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: The SGI number that MSIs start. >> + >> + brcm,msi-num-spis: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: The number of SGIs for MSIs. >> + >> + brcm,msi-offset: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Shift the allocated MSIs up by N. > > If only we had a property that every MSI controller seems to need. Go > check msi-controller.yaml... This exists because the second instance of MIP MSI-X interrupt-controller (mip1) has some limitations. Snippet from donwstream dtsi: brcm,msi-base-spi = <247>; /* Actually 20 total, but the others are * both sparse and non-consecutive */ brcm,msi-num-spis = <8>; brcm,msi-offset = <8>; brcm,msi-pci-addr = <0xff 0xffffe000>; I don't know how to model this except private property. > >> + >> + brcm,msi-pci-addr: >> + $ref: /schemas/types.yaml#/definitions/uint64 >> + description: MSI-X message address. > > Why don't other platforms need something like this? This is a destination address for MSI mem writes from PCIe endpoint devices, i.e. msi_msg.address filled when composing msi_msg (irq_chip::irq_compose_msi_msg). ~Stan > >> + >> +additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupt-controller >> + - "#interrupt-cells" >> + - msi-controller >> + >> +examples: >> + - | >> + msi-controller@130000 { >> + compatible = "brcm,bcm2712-mip-intc"; >> + reg = <0x00130000 0xc0>; >> + msi-controller; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + brcm,msi-base-spi = <128>; >> + brcm,msi-num-spis = <64>; >> + brcm,msi-offset = <0>; >> + brcm,msi-pci-addr = <0xff 0xfffff000>; >> + }; >> -- >> 2.43.0 >>
diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml new file mode 100644 index 000000000000..ca610e4467d9 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interrupt-controller/brcm,bcm2712-msix.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Broadcom bcm2712 MSI-X Interrupt Peripheral support + +maintainers: + - Stanimir Varbanov <svarbanov@suse.de> + +description: > + This interrupt controller is used to provide intterupt vectors to the + generic interrupt controller (GIC) on bcm2712. It will be used as + external MSI-X controller for PCIe root complex. + +allOf: + - $ref: /schemas/interrupt-controller/msi-controller.yaml# + +properties: + compatible: + items: + - enum: + - "brcm,bcm2712-mip-intc" + reg: + maxItems: 1 + description: > + Specifies the base physical address and size of the registers + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + msi-controller: true + + brcm,msi-base-spi: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The SGI number that MSIs start. + + brcm,msi-num-spis: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The number of SGIs for MSIs. + + brcm,msi-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Shift the allocated MSIs up by N. + + brcm,msi-pci-addr: + $ref: /schemas/types.yaml#/definitions/uint64 + description: MSI-X message address. + +additionalProperties: false + +required: + - compatible + - reg + - interrupt-controller + - "#interrupt-cells" + - msi-controller + +examples: + - | + msi-controller@130000 { + compatible = "brcm,bcm2712-mip-intc"; + reg = <0x00130000 0xc0>; + msi-controller; + interrupt-controller; + #interrupt-cells = <2>; + brcm,msi-base-spi = <128>; + brcm,msi-num-spis = <64>; + brcm,msi-offset = <0>; + brcm,msi-pci-addr = <0xff 0xfffff000>; + };
Adds DT bindings for bcm2712 MSI-X interrupt peripheral controller. Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> --- .../brcm,bcm2712-msix.yaml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2712-msix.yaml