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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

-- 
மணிவண்ணன் சதாசிவம்