| Message ID | 20231024151014.240695-4-nks@flawful.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | rk3588 PCIe improvements | expand |
On Tue, Oct 24, 2023 at 05:10:10PM +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 properties defined, in order to be > able to use these properties, while still making sure 'make CHECK_DTBS=y' > pass, we need to add these properties to rockchip-dw-pcie.yaml. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 20 +++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > index 229f8608c535..633f8e0e884f 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -35,6 +35,7 @@ properties: > - description: Rockchip designed configuration registers > - description: Config registers > - description: iATU registers > + - description: eDMA registers Same here, is this just for one of the two supported models, or for both? Cheers, Conor. > > reg-names: > minItems: 3 > @@ -43,6 +44,7 @@ properties: > - const: apb > - const: config > - const: atu > + - const: dma > > clocks: > minItems: 5 > @@ -65,6 +67,7 @@ properties: > - const: pipe > > interrupts: > + minItems: 5 > items: > - description: > Combined system interrupt, which is used to signal the following > @@ -88,14 +91,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. > -- > 2.41.0 >
Hello Conor, On Tue, Oct 24, 2023 at 05:30:22PM +0100, Conor Dooley wrote: > On Tue, Oct 24, 2023 at 05:10:10PM +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 properties defined, in order to be > > able to use these properties, while still making sure 'make CHECK_DTBS=y' > > pass, we need to add these properties to rockchip-dw-pcie.yaml. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 20 +++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > index 229f8608c535..633f8e0e884f 100644 > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -35,6 +35,7 @@ properties: > > - description: Rockchip designed configuration registers > > - description: Config registers > > - description: iATU registers > > + - description: eDMA registers > > Same here, is this just for one of the two supported models, or for > both? For the 3 controllers found in rk3568, this range exists for all (all of the controllers were synthesized with the eDMA controller). For the 5 controllers found in rk3588, this range exists for only one of them (only one of the controllers was synthesized with the eDMA controller). > > interrupt-names: > > + minItems: 5 > > items: > > - const: sys > > - const: pmc > > - const: msg > > - const: legacy > > - const: err > > + - const: dma0 > > + - const: dma1 > > + - const: dma2 > > + - const: dma3 While all the PCIe controllers on the rk3568 have the embedded DMA controller as part of the PCIe controller, they don't have separate IRQs for the eDMA. (They will need to use the combined "sys" irq, so the driver will need to read an additional register to see that it was an eDMA irq.) For the rk3588, only one of the 5 PCIe controllers have the eDMA, and that controller also has dedicated IRQs for the eDMA. (It should also be able to use the combined "sys" irq, but that would be less efficient, and AFAICT, the driver currently only works with dedicated IRQs.) Kind regards, Niklas
On Wed, Oct 25, 2023 at 10:07:02PM +0200, Niklas Cassel wrote: > Hello Conor, > > On Tue, Oct 24, 2023 at 05:30:22PM +0100, Conor Dooley wrote: > > On Tue, Oct 24, 2023 at 05:10:10PM +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 properties defined, in order to be > > > able to use these properties, while still making sure 'make CHECK_DTBS=y' > > > pass, we need to add these properties to rockchip-dw-pcie.yaml. > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > > > --- > > > .../bindings/pci/rockchip-dw-pcie.yaml | 20 +++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > index 229f8608c535..633f8e0e884f 100644 > > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > @@ -35,6 +35,7 @@ properties: > > > - description: Rockchip designed configuration registers > > > - description: Config registers > > > - description: iATU registers > > > + - description: eDMA registers > > > > Same here, is this just for one of the two supported models, or for > > both? > > For the 3 controllers found in rk3568, this range exists for all > (all of the controllers were synthesized with the eDMA controller). > > For the 5 controllers found in rk3588, this range exists for only one of them > (only one of the controllers was synthesized with the eDMA controller). > > > > > interrupt-names: > > > + minItems: 5 > > > items: > > > - const: sys > > > - const: pmc > > > - const: msg > > > - const: legacy > > > - const: err > > > + - const: dma0 > > > + - const: dma1 > > > + - const: dma2 > > > + - const: dma3 > > While all the PCIe controllers on the rk3568 have the embedded DMA controller > as part of the PCIe controller, they don't have separate IRQs for the eDMA. > (They will need to use the combined "sys" irq, so the driver will need to read > an additional register to see that it was an eDMA irq.) > > For the rk3588, only one of the 5 PCIe controllers have the eDMA, and that > controller also has dedicated IRQs for the eDMA. > (It should also be able to use the combined "sys" irq, but that would be less > efficient, and AFAICT, the driver currently only works with dedicated IRQs.) We could go with Sebastian's suggestion to define a 1MB range for "atu", see: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pci/snps%2Cdw-pcie.yaml#L76-L85 Which would allow the driver to probe if the eDMA is there or not (even if this is strictly bigger than the real ATU_CAP size, the size is still within the PCIe core's register map). That would solve the problem that some pcie controllers, with the exact same compatible, has a "dma" range while others do not. (All controllers would have a 1MB atu range, and none of them would have the dma range specified.) However, we would still have the problem that for the exact same compatible, some controllers have eDMA irqs specified in interrupts, and some do not... But perhaps having mandatory atu range (and no dma range) + optional dma irqs is better than mandatory atu range + optional dma range + optional dma irqs? (At least from a DT schema maintainability PoV.) Kind regards, Niklas
Hi Niklas On Wed, Oct 25, 2023 at 08:55:33PM +0000, Niklas Cassel wrote: > On Wed, Oct 25, 2023 at 10:07:02PM +0200, Niklas Cassel wrote: > > Hello Conor, > > > > On Tue, Oct 24, 2023 at 05:30:22PM +0100, Conor Dooley wrote: > > > On Tue, Oct 24, 2023 at 05:10:10PM +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 properties defined, in order to be > > > > able to use these properties, while still making sure 'make CHECK_DTBS=y' > > > > pass, we need to add these properties to rockchip-dw-pcie.yaml. > > > > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > > > > --- > > > > .../bindings/pci/rockchip-dw-pcie.yaml | 20 +++++++++++++++++++ > > > > 1 file changed, 20 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > > index 229f8608c535..633f8e0e884f 100644 > > > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > > @@ -35,6 +35,7 @@ properties: > > > > - description: Rockchip designed configuration registers > > > > - description: Config registers > > > > - description: iATU registers > > > > + - description: eDMA registers Are you sure the eDMA regs aren't just a part of the same space as iATU and available within the default offset DEFAULT_DBI_DMA_OFFSET? If it is then drop separate eDMA reg, having just 'atu' will be enough. > > > > > > Same here, is this just for one of the two supported models, or for > > > both? > > > > For the 3 controllers found in rk3568, this range exists for all > > (all of the controllers were synthesized with the eDMA controller). > > > > For the 5 controllers found in rk3588, this range exists for only one of them > > (only one of the controllers was synthesized with the eDMA controller). > > > > > > > > interrupt-names: > > > > + minItems: 5 > > > > items: > > > > - const: sys > > > > - const: pmc > > > > - const: msg > > > > - const: legacy > > > > - const: err > > > > + - const: dma0 > > > > + - const: dma1 > > > > + - const: dma2 > > > > + - const: dma3 > > > > While all the PCIe controllers on the rk3568 have the embedded DMA controller > > as part of the PCIe controller, they don't have separate IRQs for the eDMA. > > (They will need to use the combined "sys" irq, so the driver will need to read > > an additional register to see that it was an eDMA irq.) If it's some vendor-specific register, then the best solution would be to create a custom IRQ domain with 'sys' IRQ being a parental IRQ and with the "dma*" IRQs being the child IRQs. Then you can re-define the dw_pcie_edma_ops.irq_vector method (and edma.nr_irqs field), which would return the DMA IRQs from the new sub-domain. If it's just all the IRQs combined in a single "sys" IRQ line with no custom way to distinguish one from another, then you can re-define the dw_pcie_edma_ops.irq_vector method with a method returning the 'sys' IRQ vector (don't forget to set the edma.nr_irqs field). eDMA IRQ handler will check whether the IRQ was originated by the eDMA controller. > > > > For the rk3588, only one of the 5 PCIe controllers have the eDMA, and that > > controller also has dedicated IRQs for the eDMA. > > (It should also be able to use the combined "sys" irq, but that would be less > > efficient, and AFAICT, the driver currently only works with dedicated IRQs.) This is a standard way of the eDMA IRQs definition. So supposedly there won't be problem with rk3588 > > We could go with Sebastian's suggestion to define a 1MB range for "atu", see: > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pci/snps%2Cdw-pcie.yaml#L76-L85 Right, That's what I asked in my very first message. > > Which would allow the driver to probe if the eDMA is there or not > (even if this is strictly bigger than the real ATU_CAP size, the size is still > within the PCIe core's register map). > > That would solve the problem that some pcie controllers, with the exact same > compatible, has a "dma" range while others do not. > (All controllers would have a 1MB atu range, and none of them would have the > dma range specified.) > > However, we would still have the problem that for the exact same compatible, > some controllers have eDMA irqs specified in interrupts, and some do not... You can define the normal eDMA IRQs as optional for all Rockchip controllers (or for rk3588 only). They will be auto-detected by the DW PCIe core driver as expected if the standard eDMA IRQs are available. But for the rk3568 you'll need to redefine the dw_edma_chip.ops operation to have a custom irq_vector() callback as I described above. > > But perhaps having mandatory atu range (and no dma range) + optional dma irqs > is better than mandatory atu range + optional dma range + optional dma irqs? > (At least from a DT schema maintainability PoV.) As I mentioned in the first message, if eDMA CSRs are available within the standard offset PCIE_DMA_UNROLL_BASE with respect to the iATU CSRs base, then that is a single CSRs space (selected by the DBI_CS2=1 and CDM_SB=1 internal IP-core signals) and just 'atu' reg is supposed to be specified. -Serge(y) > > > Kind regards, > Niklas
On Tue, Oct 24, 2023 at 05:10:10PM +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 properties defined, in order to be > able to use these properties, while still making sure 'make CHECK_DTBS=y' > pass, we need to add these properties to rockchip-dw-pcie.yaml. > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 20 +++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > index 229f8608c535..633f8e0e884f 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -35,6 +35,7 @@ properties: > - description: Rockchip designed configuration registers > - description: Config registers > - description: iATU registers > + - description: eDMA registers > > reg-names: > minItems: 3 > @@ -43,6 +44,7 @@ properties: > - const: apb > - const: config > - const: atu > + - const: dma > > clocks: > minItems: 5 > @@ -65,6 +67,7 @@ properties: > - const: pipe > > interrupts: > + minItems: 5 > items: > - description: > Combined system interrupt, which is used to signal the following > @@ -88,14 +91,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. They aren't identical. Some IRQs indicate events on the write eDMA channels, some - read eDMA channels. The respective channel ID would be also useful to have in the description. -Serge(y) > > 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. > -- > 2.41.0 >
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml index 229f8608c535..633f8e0e884f 100644 --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -35,6 +35,7 @@ properties: - description: Rockchip designed configuration registers - description: Config registers - description: iATU registers + - description: eDMA registers reg-names: minItems: 3 @@ -43,6 +44,7 @@ properties: - const: apb - const: config - const: atu + - const: dma clocks: minItems: 5 @@ -65,6 +67,7 @@ properties: - const: pipe interrupts: + minItems: 5 items: - description: Combined system interrupt, which is used to signal the following @@ -88,14 +91,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.