| Message ID | 20230616170022.76107-2-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RK3588 PCIe2 support | expand |
On Fri, 16 Jun 2023 19:00:19 +0200, Sebastian Reichel wrote: > The RK356x (and RK3588) have 5 ganged interrupts. For example the > "legacy" interrupt combines "inta/intb/intc/intd" with a register > providing the details. > > Currently the binding is not specifying these interrupts resulting > in a bunch of errors for all rk356x boards using PCIe. > > Fix this by specifying the interrupts and add them to the example > to prevent regressions. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- > 2 files changed, 32 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri, Jun 16, 2023 at 07:00:19PM +0200, Sebastian Reichel wrote: > The RK356x (and RK3588) have 5 ganged interrupts. For example the > "legacy" interrupt combines "inta/intb/intc/intd" with a register > providing the details. > > Currently the binding is not specifying these interrupts resulting > in a bunch of errors for all rk356x boards using PCIe. > > Fix this by specifying the interrupts and add them to the example > to prevent regressions. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > index 24c88942e59e..98e45d2d8dfe 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -56,6 +56,17 @@ properties: > - const: pclk > - const: aux > > + interrupts: > + maxItems: 5 > + > + interrupt-names: > + items: > + - const: sys > + - const: pmc > + - const: msg > + - const: legacy > + - const: err > + > msi-map: true > > num-lanes: true > @@ -98,6 +109,7 @@ unevaluatedProperties: false > > examples: > - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > bus { > #address-cells = <2>; > @@ -117,6 +129,12 @@ examples: > "aclk_dbi", "pclk", > "aux"; > device_type = "pci"; > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > linux,pci-domain = <2>; > max-link-speed = <2>; > msi-map = <0x2000 &its 0x2000 0x1000>; > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > index 1a83f0f65f19..9f605eb297f5 100644 > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > @@ -193,9 +193,22 @@ properties: > oneOf: > - description: See native "app" IRQ for details > enum: [ intr ] The IRQs below are either combined version of the already defined IRQs or just the generic DW PCIe IRQs but named differently. Moreover I don't see kernel using any of them except the "legacy" interrupt. What about converting the dts files to using the already defined names instead of extending the already over-diverged DT-bindings? Rob, Krzysztof? In anyway in order to prevent from defining the new DW PCIe bindings compatible with your vendor-specific names please move the aliases to being under the last entry of the "interrupt-names" items property. (See the "intr" IRQ name for example or the way the vendor-specific names are defined in the reg-names property.) > + - description: Combined Legacy A/B/C/D interrupt signal. > + const: legacy This is a combined signal of "^int(a|b|c|d)$". So the entry is supposed to look: + - description: See native "int*" IRQ for details + const: legacy > + - description: Combined System interrupt signal. > + const: sys This seems like the "app" interrupt. So please either convert the dts file to using the "app" name or move this to being defined in the same entry as the "intr" name. > + - description: Combined Power Management interrupt signal. > + const: pmc This is an alias to the already defined "pme" name. So either convert the dts file to using "pme" or move this to being in the vendor-specific list of the "interrupt-names" property: + - description: See native "pme" IRQ for details + const: pmc > + - description: Combined Message Received interrupt signal. > + const: msg ditto but with respect to the "msi" name. > + - description: Combined Error interrupt signal. > + const: err ditto but with respect to the "sft_*" name. > + > allOf: > - contains: > - const: msi > + enum: > + - msi > + - msg * Please see my suggestion about converting the DTS file instead. -Serge(y) > > additionalProperties: true > > -- > 2.39.2 >
On Tue, Jun 27, 2023 at 6:27 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Fri, Jun 16, 2023 at 07:00:19PM +0200, Sebastian Reichel wrote: > > The RK356x (and RK3588) have 5 ganged interrupts. For example the > > "legacy" interrupt combines "inta/intb/intc/intd" with a register > > providing the details. > > > > Currently the binding is not specifying these interrupts resulting > > in a bunch of errors for all rk356x boards using PCIe. > > > > Fix this by specifying the interrupts and add them to the example > > to prevent regressions. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- > > 2 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > index 24c88942e59e..98e45d2d8dfe 100644 > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -56,6 +56,17 @@ properties: > > - const: pclk > > - const: aux > > > > + interrupts: > > + maxItems: 5 > > + > > + interrupt-names: > > + items: > > + - const: sys > > + - const: pmc > > + - const: msg > > + - const: legacy > > + - const: err > > + > > msi-map: true > > > > num-lanes: true > > @@ -98,6 +109,7 @@ unevaluatedProperties: false > > > > examples: > > - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > bus { > > #address-cells = <2>; > > @@ -117,6 +129,12 @@ examples: > > "aclk_dbi", "pclk", > > "aux"; > > device_type = "pci"; > > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > > linux,pci-domain = <2>; > > max-link-speed = <2>; > > msi-map = <0x2000 &its 0x2000 0x1000>; > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > index 1a83f0f65f19..9f605eb297f5 100644 > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > @@ -193,9 +193,22 @@ properties: > > oneOf: > > - description: See native "app" IRQ for details > > enum: [ intr ] > > The IRQs below are either combined version of the already defined IRQs > or just the generic DW PCIe IRQs but named differently. Moreover I > don't see kernel using any of them except the "legacy" interrupt. What > about converting the dts files to using the already defined names instead > of extending the already over-diverged DT-bindings? > Rob, Krzysztof? If there's not a dependency on the names, then we can get away with changing them. Otherwise, it's an ABI issue to change them. Supporting both names in the driver partially mitigates that, but isn't worth carrying that IMO. Will drop this one from my tree. Seems patches 2 and 3 aren't dependent on this one. Rob
On Tue, Jun 27, 2023 at 07:48:29AM -0600, Rob Herring wrote: > On Tue, Jun 27, 2023 at 6:27 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Fri, Jun 16, 2023 at 07:00:19PM +0200, Sebastian Reichel wrote: > > > The RK356x (and RK3588) have 5 ganged interrupts. For example the > > > "legacy" interrupt combines "inta/intb/intc/intd" with a register > > > providing the details. > > > > > > Currently the binding is not specifying these interrupts resulting > > > in a bunch of errors for all rk356x boards using PCIe. > > > > > > Fix this by specifying the interrupts and add them to the example > > > to prevent regressions. > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > > --- > > > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- > > > 2 files changed, 32 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > index 24c88942e59e..98e45d2d8dfe 100644 > > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > @@ -56,6 +56,17 @@ properties: > > > - const: pclk > > > - const: aux > > > > > > + interrupts: > > > + maxItems: 5 > > > + > > > + interrupt-names: > > > + items: > > > + - const: sys > > > + - const: pmc > > > + - const: msg > > > + - const: legacy > > > + - const: err > > > + > > > msi-map: true > > > > > > num-lanes: true > > > @@ -98,6 +109,7 @@ unevaluatedProperties: false > > > > > > examples: > > > - | > > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > bus { > > > #address-cells = <2>; > > > @@ -117,6 +129,12 @@ examples: > > > "aclk_dbi", "pclk", > > > "aux"; > > > device_type = "pci"; > > > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > > > + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > > > + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > > > + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > > > + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > > > linux,pci-domain = <2>; > > > max-link-speed = <2>; > > > msi-map = <0x2000 &its 0x2000 0x1000>; > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > > index 1a83f0f65f19..9f605eb297f5 100644 > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > > @@ -193,9 +193,22 @@ properties: > > > oneOf: > > > - description: See native "app" IRQ for details > > > enum: [ intr ] > > > > The IRQs below are either combined version of the already defined IRQs > > or just the generic DW PCIe IRQs but named differently. Moreover I > > don't see kernel using any of them except the "legacy" interrupt. What > > about converting the dts files to using the already defined names instead > > of extending the already over-diverged DT-bindings? > > Rob, Krzysztof? > > If there's not a dependency on the names, then we can get away with > changing them. Otherwise, it's an ABI issue to change them. Supporting > both names in the driver partially mitigates that, but isn't worth > carrying that IMO. DW Rockchip LLDD only looks for the "legacy" IRQ name. The rest of them are left unused by both LLDD and the DW PCIe core driver. So from the kernel point of view ABI is defined for "legacy" name only. Even "msi"/"msg" IRQ name left unused which is normally responsible for signaling MSI TLPs caught by the iMSI-RX engine. (Most likely MSI packets passes through the PCI<->System bus bridge and gets detected by the system GIC.) -Serge(y) > > Will drop this one from my tree. Seems patches 2 and 3 aren't > dependent on this one. > > Rob
Hi, Sorry for the delayed response. On Tue, Jun 27, 2023 at 03:27:33PM +0300, Serge Semin wrote: > On Fri, Jun 16, 2023 at 07:00:19PM +0200, Sebastian Reichel wrote: > > The RK356x (and RK3588) have 5 ganged interrupts. For example the > > "legacy" interrupt combines "inta/intb/intc/intd" with a register > > providing the details. > > > > Currently the binding is not specifying these interrupts resulting > > in a bunch of errors for all rk356x boards using PCIe. > > > > Fix this by specifying the interrupts and add them to the example > > to prevent regressions. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- > > 2 files changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > index 24c88942e59e..98e45d2d8dfe 100644 > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -56,6 +56,17 @@ properties: > > - const: pclk > > - const: aux > > > > + interrupts: > > + maxItems: 5 > > + > > + interrupt-names: > > + items: > > + - const: sys > > + - const: pmc > > + - const: msg > > + - const: legacy > > + - const: err > > + > > msi-map: true > > > > num-lanes: true > > @@ -98,6 +109,7 @@ unevaluatedProperties: false > > > > examples: > > - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > bus { > > #address-cells = <2>; > > @@ -117,6 +129,12 @@ examples: > > "aclk_dbi", "pclk", > > "aux"; > > device_type = "pci"; > > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > > linux,pci-domain = <2>; > > max-link-speed = <2>; > > msi-map = <0x2000 &its 0x2000 0x1000>; > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > index 1a83f0f65f19..9f605eb297f5 100644 > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > @@ -193,9 +193,22 @@ properties: > > oneOf: > > - description: See native "app" IRQ for details > > enum: [ intr ] > > The IRQs below are either combined version of the already defined IRQs > or just the generic DW PCIe IRQs but named differently. Moreover I > don't see kernel using any of them except the "legacy" interrupt. What > about converting the dts files to using the already defined names instead > of extending the already over-diverged DT-bindings? > Rob, Krzysztof? > > In anyway in order to prevent from defining the new DW PCIe bindings > compatible with your vendor-specific names please move the aliases to > being under the last entry of the "interrupt-names" items property. > (See the "intr" IRQ name for example or the way the vendor-specific > names are defined in the reg-names property.) All of these are combined interrupts and not simple aliases. Otherwise I would just have changed the name for RK3588. Rockchip has a two level interrupt system. I re-checked carefully and as far as I can tell all interrupts currently defined in the binding have a specific meaning. This is not the case for the interrupts from RK3588. I will send a new version in a jiffy, which describes all the sub-IRQs available beneath the newly described ones. I don't have the Synopsys datasheet, so I will stick to the names used by Rockchip. > > + - description: Combined Legacy A/B/C/D interrupt signal. > > + const: legacy > > This is a combined signal of "^int(a|b|c|d)$". So the entry > is supposed to look: > + - description: See native "int*" IRQ for details > + const: legacy In case my explanation from above was not clear: All the other interrupts follow the same style as this one. > > + - description: Combined System interrupt signal. > > + const: sys > > This seems like the "app" interrupt. So please either convert the dts > file to using the "app" name or move this to being defined in the same > entry as the "intr" name. I suppose "sys", "pmc", "msg" and "err" all fit for "app", since they are vendor specific with the extra layer? But obviously I cannot specify "app" more than once. > > + - description: Combined Power Management interrupt signal. > > + const: pmc > > This is an alias to the already defined "pme" name. So either convert > the dts file to using "pme" or move this to being in the > vendor-specific list of the "interrupt-names" property: > + - description: See native "pme" IRQ for details > + const: pmc pme should be 'msg -> pm_pme_int': Interrupt indicates that the controller received a PM_PME message. > > + - description: Combined Message Received interrupt signal. > > + const: msg > > ditto but with respect to the "msi" name. MSI is handled via GIC-ITS using this DT property: msi-map = <0x3000 &its0 0x3000 0x1000>; > > + - description: Combined Error interrupt signal. > > + const: err > > ditto but with respect to the "sft_*" name. I really meant it, when I wrote "Combined". Appart from (un)correctable errors this also reports e.g. timeouts. > > + > > allOf: > > - contains: > > - const: msi > > + enum: > > + - msi > > + - msg > > * Please see my suggestion about converting the DTS file instead. My understanding is, that "msi" and "msg" are not the same. MSI is an interrupt message from a peripheral device, but "msg" is a combined interrupt for all kind of messages. -- Sebastian
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml index 24c88942e59e..98e45d2d8dfe 100644 --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -56,6 +56,17 @@ properties: - const: pclk - const: aux + interrupts: + maxItems: 5 + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + msi-map: true num-lanes: true @@ -98,6 +109,7 @@ unevaluatedProperties: false examples: - | + #include <dt-bindings/interrupt-controller/arm-gic.h> bus { #address-cells = <2>; @@ -117,6 +129,12 @@ examples: "aclk_dbi", "pclk", "aux"; device_type = "pci"; + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; linux,pci-domain = <2>; max-link-speed = <2>; msi-map = <0x2000 &its 0x2000 0x1000>; diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml index 1a83f0f65f19..9f605eb297f5 100644 --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml @@ -193,9 +193,22 @@ properties: oneOf: - description: See native "app" IRQ for details enum: [ intr ] + - description: Combined Legacy A/B/C/D interrupt signal. + const: legacy + - description: Combined System interrupt signal. + const: sys + - description: Combined Power Management interrupt signal. + const: pmc + - description: Combined Message Received interrupt signal. + const: msg + - description: Combined Error interrupt signal. + const: err + allOf: - contains: - const: msi + enum: + - msi + - msg additionalProperties: true
The RK356x (and RK3588) have 5 ganged interrupts. For example the "legacy" interrupt combines "inta/intb/intc/intd" with a register providing the details. Currently the binding is not specifying these interrupts resulting in a bunch of errors for all rk356x boards using PCIe. Fix this by specifying the interrupts and add them to the example to prevent regressions. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ .../devicetree/bindings/pci/snps,dw-pcie.yaml | 15 ++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-)