Message ID | 20231027145422.40265-3-nks@flawful.org |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | rockchip DWC PCIe improvements | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/patch-applied | success | |
robh/dtbs-check | warning | build log |
robh/dt-meta-schema | success |
On Fri, 27 Oct 2023 16:54:14 +0200, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > using: > > allOf: > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > and snps,dw-pcie.yaml does have the dma interrupts defined, in order to be > able to use these interrupts, while still making sure 'make CHECK_DTBS=y' > pass, we need to add these interrupts to rockchip-dw-pcie.yaml. > > These dedicated interrupts for the eDMA are not always wired to all the > PCIe controllers on the same SoC. In other words, even for a specific > compatible, e.g. rockchip,rk3588-pcie, these dedicated eDMA interrupts > may or may not be wired. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri, Oct 27, 2023 at 04:54:14PM +0200, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > using: > > allOf: > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > and snps,dw-pcie.yaml does have the dma interrupts defined, in order to be > able to use these interrupts, while still making sure 'make CHECK_DTBS=y' > pass, we need to add these interrupts to rockchip-dw-pcie.yaml. > > These dedicated interrupts for the eDMA are not always wired to all the > PCIe controllers on the same SoC. In other words, even for a specific > compatible, e.g. rockchip,rk3588-pcie, these dedicated eDMA interrupts > may or may not be wired. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > index 6ca87ff4ae20..7ad6e1283784 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -63,6 +63,7 @@ properties: > - const: pipe > > interrupts: > + minItems: 5 > items: > - description: > Combined system interrupt, which is used to signal the following > @@ -86,14 +87,31 @@ properties: > interrupts - aer_rc_err, aer_rc_err_msi, rx_cpl_timeout, > tx_cpl_timeout, cor_err_sent, nf_err_sent, f_err_sent, cor_err_rx, > nf_err_rx, f_err_rx, radm_qoverflow > + - description: > + Indicates that the eDMA Tx/Rx transfer is complete or that an > + error has occurred on the corresponding channel. > + - description: > + Indicates that the eDMA Tx/Rx transfer is complete or that an > + error has occurred on the corresponding channel. > + - description: > + Indicates that the eDMA Tx/Rx transfer is complete or that an > + error has occurred on the corresponding channel. > + - description: > + Indicates that the eDMA Tx/Rx transfer is complete or that an > + error has occurred on the corresponding channel. > > interrupt-names: > + minItems: 5 > items: > - const: sys > - const: pmc > - const: msg > - const: legacy > - const: err > + - const: dma0 > + - const: dma1 > + - const: dma2 > + - const: dma3 No. As you said yourself here https://lore.kernel.org/linux-pci/ZTl1ZsHvk3DDHWsm@x1-carbon/ and in the response to my last message in v2, which for some reason hasn't got to the lore archive: On Fri, Oct 27, 2023 at 05:51:14PM +0200, Niklas Cassel wrote: > However, e.g. rk3568 only has one channel for reads and one for writes. > (Now this SoC doesn't have dedicated IRQs for the eDMA, but let's pretend > that it did.) > > So for rk3568, it would then instead be: > dma0: wr0 > dma1: rd0 > dma2: <unused> > dma3: <unused> rk3568 doesn't have IRQs supplied in a normal way, as separate signals. Instead they are combined in the 'sys' IRQ. So you should define the IRQs constraint being device-specific by using for example the "allOf: if-else" pattern. -Serge(y) > > legacy-interrupt-controller: > description: Interrupt controller node for handling legacy PCI interrupts. > -- > 2.41.0 >
On Tue, Oct 31, 2023 at 04:10:17AM +0300, Serge Semin wrote: > On Fri, Oct 27, 2023 at 05:51:14PM +0200, Niklas Cassel wrote: > > However, e.g. rk3568 only has one channel for reads and one for writes. > > (Now this SoC doesn't have dedicated IRQs for the eDMA, but let's pretend > > that it did.) > > > > So for rk3568, it would then instead be: > > dma0: wr0 > > dma1: rd0 > > dma2: <unused> > > dma3: <unused> > > rk3568 doesn't have IRQs supplied in a normal way, as separate > signals. Instead they are combined in the 'sys' IRQ. So you should > define the IRQs constraint being device-specific by using for example > the "allOf: if-else" pattern. Thank you for your review comment. I agree. Will fix this in next version. Kind regards, Niklas
On Tue, Oct 31, 2023 at 03:51:03PM +0000, Niklas Cassel wrote: > On Tue, Oct 31, 2023 at 04:10:17AM +0300, Serge Semin wrote: > > On Fri, Oct 27, 2023 at 05:51:14PM +0200, Niklas Cassel wrote: > > > However, e.g. rk3568 only has one channel for reads and one for writes. > > > (Now this SoC doesn't have dedicated IRQs for the eDMA, but let's pretend > > > that it did.) > > > > > > So for rk3568, it would then instead be: > > > dma0: wr0 > > > dma1: rd0 > > > dma2: <unused> > > > dma3: <unused> > > > > rk3568 doesn't have IRQs supplied in a normal way, as separate > > signals. Instead they are combined in the 'sys' IRQ. So you should > > define the IRQs constraint being device-specific by using for example > > the "allOf: if-else" pattern. > > Thank you for your review comment. > > I agree. Will fix this in next version. When you do, would you mind capitalizing "ATU", "DMA", etc in your subject lines, commit logs, comments, etc? Then it'll be more obvious that these aren't ordinary English words. Bjorn
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml index 6ca87ff4ae20..7ad6e1283784 100644 --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -63,6 +63,7 @@ properties: - const: pipe interrupts: + minItems: 5 items: - description: Combined system interrupt, which is used to signal the following @@ -86,14 +87,31 @@ properties: interrupts - aer_rc_err, aer_rc_err_msi, rx_cpl_timeout, tx_cpl_timeout, cor_err_sent, nf_err_sent, f_err_sent, cor_err_rx, nf_err_rx, f_err_rx, radm_qoverflow + - description: + Indicates that the eDMA Tx/Rx transfer is complete or that an + error has occurred on the corresponding channel. + - description: + Indicates that the eDMA Tx/Rx transfer is complete or that an + error has occurred on the corresponding channel. + - description: + Indicates that the eDMA Tx/Rx transfer is complete or that an + error has occurred on the corresponding channel. + - description: + Indicates that the eDMA Tx/Rx transfer is complete or that an + error has occurred on the corresponding channel. interrupt-names: + minItems: 5 items: - const: sys - const: pmc - const: msg - const: legacy - const: err + - const: dma0 + - const: dma1 + - const: dma2 + - const: dma3 legacy-interrupt-controller: description: Interrupt controller node for handling legacy PCI interrupts.