diff mbox series

[v4,03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

Message ID 20240529-rockchip-pcie-ep-v1-v4-3-3dc00fe21a78@kernel.org (mailing list archive)
State New
Headers show
Series PCI: dw-rockchip: Add endpoint mode support | expand

Commit Message

Niklas Cassel May 29, 2024, 8:28 a.m. UTC 
The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
that are triggered when the PCIe controller (when running in Endpoint mode)
has sent an Assert_INTA Message to the upstream device.

Some DWC controllers have these interrupt in a combined interrupt signal.

Add the description of these interrupts to the device tree binding.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Manivannan Sadhasivam June 5, 2024, 7:34 a.m. UTC | #1 
On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> that are triggered when the PCIe controller (when running in Endpoint mode)
> has sent an Assert_INTA Message to the upstream device.
> 
> Some DWC controllers have these interrupt in a combined interrupt signal.
> 
> Add the description of these interrupts to the device tree binding.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
RC binding recently (ebce9f6623a7), should we rename it?

This will force the driver to support both 'legacy' and 'intx' for backwards
compatibility.

But irrespective of that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> index f5f12cbc2cb3..f474b9e3fc7e 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> @@ -151,6 +151,15 @@ properties:
>              Application-specific IRQ raised depending on the vendor-specific
>              events basis.
>            const: app
> +        - description:
> +            Interrupts triggered when the controller itself (in Endpoint mode)
> +            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
> +            the upstream device.
> +          pattern: "^tx_int(a|b|c|d)$"
> +        - description:
> +            Combined interrupt signal raised when the controller has sent an
> +            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
> +          const: legacy
>          - description:
>              Vendor-specific IRQ names. Consider using the generic names above
>              for new bindings.
> 
> -- 
> 2.45.1
>
Niklas Cassel June 5, 2024, 4:20 p.m. UTC | #2 
On Wed, Jun 05, 2024 at 01:04:02PM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> > The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> > that are triggered when the PCIe controller (when running in Endpoint mode)
> > has sent an Assert_INTA Message to the upstream device.
> >
> > Some DWC controllers have these interrupt in a combined interrupt signal.
> >
> > Add the description of these interrupts to the device tree binding.
> >
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
>
> Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
> binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
> RC binding recently (ebce9f6623a7), should we rename it?
>
> This will force the driver to support both 'legacy' and 'intx' for backwards
> compatibility.

I don't think this is true.


Look at snps,dw-pcie.yaml in 6.10-rc2:

The individual interrupts are called:
            Legacy A/B/C/D interrupt signal. Basically it's triggered by
            receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message
            from the downstream device.
          pattern: "^int(a|b|c|d)$"

The combined interrupt is called:
            Combined Legacy A/B/C/D interrupt signal. See "^int(a|b|c|d)$" for
            details.
          const: legacy

So you use 'inta', 'intb', 'intc', 'intd' if your SoC has a dedicated
interrupt line for each of these irqs.

If the SoC simply has a single combined interrupt line for these irqs,
then you use 'legacy'


This patch simply adds:
'tx_inta', 'tx_intb', 'tx_intc', 'tx_intd' as individual interrupts
and the combined interrupt 'legacy' to snps,dw-pcie-ep.yaml.


Patch ebce9f6623a7 simply allowed the combined interrupt line 'legacy'
to be used by the rockchip-dw-pcie.yaml binding.
This is because the way that device tree is designed. You need to specify
something both in the generic binding (which specifies everything),
and in the glue driver binding, to specify the subset that is allowed by
the glue driver.


Since a controller cannot run in both EP and RC mode at the same time,
I think that it is fine that this patch reuses the name 'legacy' for the
combined interrupt.

And as you can see in patch 5 in this series, rk3588 actually uses a single
combined IRQ (called legacy) for 'inta', 'intb', 'intc', 'intd', 'tx_inta',
'tx_intb', 'tx_intc', 'tx_intd'.


Kind regards,
Niklas


>
> But irrespective of that,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> - Mani
>
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > index f5f12cbc2cb3..f474b9e3fc7e 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > @@ -151,6 +151,15 @@ properties:
> >              Application-specific IRQ raised depending on the vendor-specific
> >              events basis.
> >            const: app
> > +        - description:
> > +            Interrupts triggered when the controller itself (in Endpoint mode)
> > +            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
> > +            the upstream device.
> > +          pattern: "^tx_int(a|b|c|d)$"
> > +        - description:
> > +            Combined interrupt signal raised when the controller has sent an
> > +            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
> > +          const: legacy
> >          - description:
> >              Vendor-specific IRQ names. Consider using the generic names above
> >              for new bindings.
> >
> > --
> > 2.45.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
Manivannan Sadhasivam June 6, 2024, 6:25 a.m. UTC | #3 
On Wed, Jun 05, 2024 at 06:20:58PM +0200, Niklas Cassel wrote:
> On Wed, Jun 05, 2024 at 01:04:02PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> > > The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> > > that are triggered when the PCIe controller (when running in Endpoint mode)
> > > has sent an Assert_INTA Message to the upstream device.
> > >
> > > Some DWC controllers have these interrupt in a combined interrupt signal.
> > >
> > > Add the description of these interrupts to the device tree binding.
> > >
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> >
> > Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
> > binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
> > RC binding recently (ebce9f6623a7), should we rename it?
> >
> > This will force the driver to support both 'legacy' and 'intx' for backwards
> > compatibility.
> 
> I don't think this is true.
> 
> 
> Look at snps,dw-pcie.yaml in 6.10-rc2:
> 
> The individual interrupts are called:
>             Legacy A/B/C/D interrupt signal. Basically it's triggered by
>             receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message
>             from the downstream device.
>           pattern: "^int(a|b|c|d)$"
> 
> The combined interrupt is called:
>             Combined Legacy A/B/C/D interrupt signal. See "^int(a|b|c|d)$" for
>             details.
>           const: legacy
> 
> So you use 'inta', 'intb', 'intc', 'intd' if your SoC has a dedicated
> interrupt line for each of these irqs.
> 
> If the SoC simply has a single combined interrupt line for these irqs,
> then you use 'legacy'
> 
> 
> This patch simply adds:
> 'tx_inta', 'tx_intb', 'tx_intc', 'tx_intd' as individual interrupts
> and the combined interrupt 'legacy' to snps,dw-pcie-ep.yaml.
> 
> 
> Patch ebce9f6623a7 simply allowed the combined interrupt line 'legacy'
> to be used by the rockchip-dw-pcie.yaml binding.
> This is because the way that device tree is designed. You need to specify
> something both in the generic binding (which specifies everything),
> and in the glue driver binding, to specify the subset that is allowed by
> the glue driver.
> 
> 
> Since a controller cannot run in both EP and RC mode at the same time,
> I think that it is fine that this patch reuses the name 'legacy' for the
> combined interrupt.
> 
> And as you can see in patch 5 in this series, rk3588 actually uses a single
> combined IRQ (called legacy) for 'inta', 'intb', 'intc', 'intd', 'tx_inta',
> 'tx_intb', 'tx_intc', 'tx_intd'.
> 

I think you misunderstood what I was asking. I was just asking if we still want
to keep the term 'legacy' for INTx IRQs in DT binding or not, since we recently
got rid of that terminology in PCI drivers.

But if the rockchip TRM defines it as 'legacy' then it should be called as is in
the rockchip binding. But I don't think DWC Spec also defines it that way (I
haven't checked).

It is a question for Rob and Bjorn.

- Mani
Niklas Cassel June 7, 2024, 9:49 a.m. UTC | #4 
On Thu, Jun 06, 2024 at 11:55:38AM +0530, Manivannan Sadhasivam wrote:
> 
> I think you misunderstood what I was asking. I was just asking if we still want
> to keep the term 'legacy' for INTx IRQs in DT binding or not, since we recently
> got rid of that terminology in PCI drivers.

I still don't think that I understand :)

In snps,dw-pcie.yaml we currently (6.10-rc2) have:

const: legacy
(for the combined IRQ)

pattern: "^int(a|b|c|d)$"
(for the individual IRQs)

So we will need to support these indefinitely.

What is it that you would want to rename?

the combined irq?

Doesn't sound like a good idea to me, as we would need to support two
(perhaps that is what you meant).

But even if you wanted to rename it, it would be hard to come up with
a name. Perhaps intx, but that would be super confusing since we already
have inta, intb, intc, intd.

I think it is best just to not touch the binding.

In kernel macros could (that have already been renamed from legacy to intx)
doesn't really have anything to do with the DT binding IMO.


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index f5f12cbc2cb3..f474b9e3fc7e 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -151,6 +151,15 @@  properties:
             Application-specific IRQ raised depending on the vendor-specific
             events basis.
           const: app
+        - description:
+            Interrupts triggered when the controller itself (in Endpoint mode)
+            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
+            the upstream device.
+          pattern: "^tx_int(a|b|c|d)$"
+        - description:
+            Combined interrupt signal raised when the controller has sent an
+            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
+          const: legacy
         - description:
             Vendor-specific IRQ names. Consider using the generic names above
             for new bindings.