Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 16:03:04 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Rob Herring <robh@kernel.org>
Cc:     Sebastian Reichel <sebastian.reichel@collabora.com>,
        linux-pci@vger.kernel.org, linux-rockchip@lists.infradead.org,
        Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Simon Xue <xxm@rock-chips.com>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        kernel@collabora.com
Subject: Re: [PATCH v3 2/5] dt-bindings: PCI: dwc: rockchip: Fix
 interrupt-names issue
Message-ID: <epqbt5c4vxb65l47tupmogxfnskwbd54cg6vzq7xnelrt44omu@py7ntcft6dhf>
References: <20230717172651.64324-1-sebastian.reichel@collabora.com>
 <20230717172651.64324-3-sebastian.reichel@collabora.com>
 <20230719201605.GA706263-robh@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230719201605.GA706263-robh@kernel.org>
Precedence: bulk

On Wed, Jul 19, 2023 at 02:16:05PM -0600, Rob Herring wrote:
> On Mon, Jul 17, 2023 at 07:26:48PM +0200, Sebastian Reichel wrote:
> > The RK356x (and RK3588) have 5 ganged interrupts. For example the
> > "legacy" interrupt combines "inta/intb/intc/intd" with a register
> > providing the details.
> > 
> > Currently the binding is not specifying these interrupts resulting
> > in a bunch of errors for all rk356x boards using PCIe.
> > 
> > Fix this by specifying the interrupts and add them to the example
> > to prevent regressions.
> > 
> > This changes the reference from snps,dw-pcie.yaml to
> > snps,dw-pcie-common.yaml, since the interrupts are vendor
> > specific and should not be listed in the generic file. The
> > only other bit from the generic binding are the reg-names,
> > which are overwritten by this binding.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../bindings/pci/rockchip-dw-pcie.yaml        | 43 ++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > index a4f61ced5e88..7836b9a5547c 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -17,7 +17,8 @@ description: |+
> >    snps,dw-pcie.yaml.
> >  
> >  allOf:
> > -  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#
> >  
> >  properties:
> >    compatible:
> > @@ -60,6 +61,39 @@ properties:
> >        - const: aux
> >        - const: pipe
> >  
> > +  interrupts:
> > +    items:
> > +      - description:
> > +          Combined system interrupt, which is used to signal the following
> > +          interrupts - phy_link_up, dll_link_up, link_req_rst_not, hp_pme,
> > +          hp, hp_msi, link_auto_bw, link_auto_bw_msi, bw_mgt, bw_mgt_msi,
> > +          edma_wr, edma_rd, dpa_sub_upd, rbar_update, link_eq_req, ep_elbi_app
> > +      - description:
> > +          Combined PM interrupt, which is used to signal the following
> > +          interrupts - linkst_in_l1sub, linkst_in_l1, linkst_in_l2,
> > +          linkst_in_l0s, linkst_out_l1sub, linkst_out_l1, linkst_out_l2,
> > +          linkst_out_l0s, pm_dstate_update
> > +      - description:
> > +          Combined message interrupt, which is used to signal the following
> > +          interrupts - ven_msg, unlock_msg, ltr_msg, cfg_pme, cfg_pme_msi,
> > +          pm_pme, pm_to_ack, pm_turnoff, obff_idle, obff_obff, obff_cpu_active
> > +      - description:
> > +          Combined legacy interrupt, which is used to signal the following
> > +          interrupts - inta, intb, intc, intd
> > +      - description:
> > +          Combined error interrupt, which is used to signal the following
> > +          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
> 

> I'm confused. It is really up to the integrator on how each of these 
> interrupts are combined? I thought it was a bit more fixed than that.

As I explained it here [1] in details there are only several signals
which are actually marked as IRQs:
hp_pme,
hp/hp_msi,
link_auto_bw/link_auto_bw_msi,
bw_mgt/bw_mgt_msi,
edma_wr/edma_rd,
cfg_pme/cfg_pme_msi,
inta, intb, intc, intd,
aer_rc_err/aer_rc_err_msi.
(not listed above: cfg_int, cfg_safety_corr, cfg_safety_uncorr,
cfg_vpd, msi_ctrl_int).
All of the above (except msi_ctrl_int, which belongs to a none-SII
group of signals) are a part of the so called "SII: Interrupt
Signals". They are normally used to indicate IRQs in the most of the
DW PCIe devices. That's why I listed them in the generic DW PCIe
DT-bindings.

The rest of the signals described by Sebastian are also a part of the
System Information Interface (SII), but they _aren't_ marked as the
IRQs. Although the signals explicitly stated as interrupts and some
common SII Signals are marked as _outputs_ (from the DW PCIe
controller point of view). So all of them can be used as the interrupt
sources if they are somehow connected to a system interrupt
controller. Though normally the none-IRQ outputs are just wired to the
application-specific CSR space and if needed tracked by a separate
IRQ signal (see the "app" signal in the generic DW PCIe DT-bindings)

AFAIU RK rk356x HW designers just grouped some SII output signals and
OR'ed them up before attaching to the system IRQ controller. So
basically all the IRQs described by Sebastian can be categorized as
application-specific IRQs. That's amongst various solutions I
suggested to rename them to indicate that (see [1]). I don't know what
the Rockchip HW-engineers were thinking providing such a mix of the
IRQ sources instead just using the standardized by Synopsys interface,
but here it is.

[1] https://lore.kernel.org/linux-pci/3628628.VLH7GnMWUR@phil/T/#m3b3149c26b15e03686cfc2b76033c9949b0d565c

-Serge(y)

> 
> Rob