[PATCH v2 0/4] rk3588 PCIe improvements

[PATCH v2 0/4] rk3588 PCIe improvements

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hello,

This series fixes two issues related to the pcie3x4 slot on the rk3588:
1) Adds the atu region, so that the driver can properly detect all 16
inbound iATUs and all 16 outbound iATUs.
2) Adds the dma region, and the related IRQs used by the eDMA, so that it
is possible to offload data transfers using the embedded DMA controller.


Changes since v1:
-Added patches to rockchip-dw-pcie.yaml to make 'make CHECK_DTBS=y' pass.


Kind regards,
Niklas

Niklas Cassel (4):
  dt-bindings: PCI: dwc: rockchip: Add atu property
  arm64: dts: rockchip: add missing mandatory rk3588 PCIe atu property
  dt-bindings: PCI: dwc: rockchip: Add dma properties
  arm64: dts: rockchip: add missing rk3588 PCIe dma properties

 .../bindings/pci/rockchip-dw-pcie.yaml        | 24 ++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3588.dtsi      | 31 ++++++++++++-------
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 14 +++++----
 3 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.41.0

[PATCH v2 1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

[Niklas Cassel]

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 atu property defined, in order to be
able to use this property, while still making sure 'make CHECK_DTBS=y'
pass, we need to add this property to rockchip-dw-pcie.yaml.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index 1ae8dcfa072c..229f8608c535 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -29,16 +29,20 @@ properties:
           - const: rockchip,rk3568-pcie
 
   reg:
+    minItems: 3
     items:
       - description: Data Bus Interface (DBI) registers
       - description: Rockchip designed configuration registers
       - description: Config registers
+      - description: iATU registers
 
   reg-names:
+    minItems: 3
     items:
       - const: dbi
       - const: apb
       - const: config
+      - const: atu
 
   clocks:
     minItems: 5
-- 
2.41.0

[PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Niklas Cassel]

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.
 
   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

Re: [PATCH v2 1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

On Tue, Oct 24, 2023 at 05:10:08PM +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 atu property defined, in order to be
> able to use this property, while still making sure 'make CHECK_DTBS=y'
> pass, we need to add this property to rockchip-dw-pcie.yaml.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 1ae8dcfa072c..229f8608c535 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -29,16 +29,20 @@ properties:
>            - const: rockchip,rk3568-pcie
>  
>    reg:
> +    minItems: 3
>      items:
>        - description: Data Bus Interface (DBI) registers
>        - description: Rockchip designed configuration registers
>        - description: Config registers
> +      - description: iATU registers

Is this extra register only for the ..88 or for the ..68 and for the
..88 models?

>  
>    reg-names:
> +    minItems: 3
>      items:
>        - const: dbi
>        - const: apb
>        - const: config
> +      - const: atu
>  
>    clocks:
>      minItems: 5
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

[Niklas Cassel]

Hello Conor,

On Tue, Oct 24, 2023 at 05:29:28PM +0100, Conor Dooley wrote:
> On Tue, Oct 24, 2023 at 05:10:08PM +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 atu property defined, in order to be
> > able to use this property, while still making sure 'make CHECK_DTBS=y'
> > pass, we need to add this property to rockchip-dw-pcie.yaml.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > index 1ae8dcfa072c..229f8608c535 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -29,16 +29,20 @@ properties:
> >            - const: rockchip,rk3568-pcie
> >  
> >    reg:
> > +    minItems: 3
> >      items:
> >        - description: Data Bus Interface (DBI) registers
> >        - description: Rockchip designed configuration registers
> >        - description: Config registers
> > +      - description: iATU registers
> 
> Is this extra register only for the ..88 or for the ..68 and for the
> ..88 models?

Looking at the rk3568 Technical Reference Manual (TRM):
https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf

The iATU register register range exists for all 3 PCIe controllers
found on the rk3568.

This register range is currently not defined in the rk3568.dtsi, so the driver
will currently use the default register offset (which is correct), but with
the driver fallback register size that is only big enough to cover 8 inbound
and 8 outbound iATUs (internal Address Translation Units).

According to the TRM, all three PCIe controllers found on the rk3568 have
16 inbound iATUs and 16 outbound iATUs, so if someone wants to be able to
make use of all the iATUs on the rk3568, they will need to add "atu" to
rk3568.dtsi.

I don't have the board, so I can't test, but I could add a patch that adds
the "atu" range for the controllers in rk3568, if that makes things easier.


Kind regards,
Niklas

Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Niklas Cassel]

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

Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Niklas Cassel]

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

Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Serge Semin]

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

Re: [PATCH v2 3/4] dt-bindings: PCI: dwc: rockchip: Add dma properties

[Serge Semin]

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
> 

Re: [PATCH v2 1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

[Rob Herring]

On Tue, Oct 24, 2023 at 05:10:08PM +0200, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>

The subject says 'atu property', but 'atu' is not a property.

> 
> 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 atu property defined, in order to be
> able to use this property, while still making sure 'make CHECK_DTBS=y'
> pass, we need to add this property to rockchip-dw-pcie.yaml.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 1ae8dcfa072c..229f8608c535 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -29,16 +29,20 @@ properties:
>            - const: rockchip,rk3568-pcie
>  
>    reg:
> +    minItems: 3
>      items:
>        - description: Data Bus Interface (DBI) registers
>        - description: Rockchip designed configuration registers
>        - description: Config registers
> +      - description: iATU registers
>  
>    reg-names:
> +    minItems: 3
>      items:
>        - const: dbi
>        - const: apb
>        - const: config
> +      - const: atu
>  
>    clocks:
>      minItems: 5
> -- 
> 2.41.0
> 

Re: [PATCH v2 1/4] dt-bindings: PCI: dwc: rockchip: Add atu property

[Rob Herring]

On Wed, Oct 25, 2023 at 08:02:32PM +0000, Niklas Cassel wrote:
> Hello Conor,
> 
> On Tue, Oct 24, 2023 at 05:29:28PM +0100, Conor Dooley wrote:
> > On Tue, Oct 24, 2023 at 05:10:08PM +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 atu property defined, in order to be
> > > able to use this property, while still making sure 'make CHECK_DTBS=y'
> > > pass, we need to add this property to rockchip-dw-pcie.yaml.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > ---
> > >  Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > index 1ae8dcfa072c..229f8608c535 100644
> > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > @@ -29,16 +29,20 @@ properties:
> > >            - const: rockchip,rk3568-pcie
> > >  
> > >    reg:
> > > +    minItems: 3
> > >      items:
> > >        - description: Data Bus Interface (DBI) registers
> > >        - description: Rockchip designed configuration registers
> > >        - description: Config registers
> > > +      - description: iATU registers
> > 
> > Is this extra register only for the ..88 or for the ..68 and for the
> > ..88 models?
> 
> Looking at the rk3568 Technical Reference Manual (TRM):
> https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf
> 
> The iATU register register range exists for all 3 PCIe controllers
> found on the rk3568.
> 
> This register range is currently not defined in the rk3568.dtsi, so the driver
> will currently use the default register offset (which is correct), but with
> the driver fallback register size that is only big enough to cover 8 inbound
> and 8 outbound iATUs (internal Address Translation Units).

We should probably make the driver smarter instead or in addition. We 
have the DBI size, Just make atu_size = dbi_size - DEFAULT_DBI_ATU_OFFSET.

> According to the TRM, all three PCIe controllers found on the rk3568 have
> 16 inbound iATUs and 16 outbound iATUs, so if someone wants to be able to
> make use of all the iATUs on the rk3568, they will need to add "atu" to
> rk3568.dtsi.

At least for host side, the number of regions used is based on ranges. 
You'd be hard pressed to need more than 8. That or no h/w with 16 is 
probably why I said 8 was enough at the time.

Rob