[v3,2/6] dt-bindings: PCI: dwc: rockchip: Add optional dma interrupts

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(+)

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