| Message ID | 20230717172651.64324-3-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Rob Herring |
| Headers | show |
| Series | RK3588 PCIe2 support | expand |
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. Rob
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
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 + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + msi-map: true num-lanes: true @@ -108,6 +142,7 @@ unevaluatedProperties: false examples: - | + #include <dt-bindings/interrupt-controller/arm-gic.h> bus { #address-cells = <2>; @@ -127,6 +162,12 @@ examples: "aclk_dbi", "pclk", "aux"; device_type = "pci"; + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; linux,pci-domain = <2>; max-link-speed = <2>; msi-map = <0x2000 &its 0x2000 0x1000>;
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(-)