Re: [PATCH v2 1/3] dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue

[Serge Semin]

Hi Sebastian

On Thu, Jul 13, 2023 at 07:18:49PM +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.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 18 +++++
>  .../devicetree/bindings/pci/snps,dw-pcie.yaml | 76 ++++++++++++++++++-
>  2 files changed, 93 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..aad53c7d8485 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -60,6 +60,17 @@ properties:
>        - const: aux
>        - const: pipe
>  
> +  interrupts:
> +    maxItems: 5
> +
> +  interrupt-names:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +
>    msi-map: true
>  
>    num-lanes: true
> @@ -108,6 +119,7 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>      bus {
>          #address-cells = <2>;
> @@ -127,6 +139,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>;
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 1a83f0f65f19..973bf8f2730d 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -193,9 +193,83 @@ properties:
>            oneOf:
>              - description: See native "app" IRQ for details
>                enum: [ intr ]
> +        - description:
> +            Combined legacy interrupt, which is used to signal the following
> +            interrupts
> +              * inta
> +              * intb
> +              * intc
> +              * intd
> +          const: legacy
> +        - 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
> +          const: sys
> +        - 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
> +          const: pmc
> +        - 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

These are marked is "inputs" (from the DW PCIe controller point of
view) in the HW manual. Are you sure they are supposed to generate any
IRQ? Based on the DW PCIe HW-manual they are supposed to be set by the
_application_ (a.k.a your driver or vendor-specific RTL block) as a
request to the DW PCIe controller to emit an OBFF message. There is a
signal marked as "output" and named as "app_obff_msg_grant" which most
likely is relevant here.

> +          const: msg
> +        - 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
> +          const: err

The most of the signals you cited in the description properties are a
part of the so called "System Information Interface" defined in the DW
PCIe databook. Here is what the doc says regarding these signals:

"The SII exchanges various system-related information between the
controller and your application. Most of the SII signals are provided
for flexibility. Your application is not required to use all of the
SII signals. Your application logic is expected to drive and monitor
the signals that it needs to function correctly. SII inputs that your
application does not require, must be driven to 0."

Amongst tons of various informational signals available in the
framework of SII, there is "SII: Interrupt Signals" which are normally
utilized by the vendor-specific controller implementations and which
are defined as generic in this DT-bindings. (MSI IRQ signal is defined
separately from SII as "MSI Interface Signals".)

What is normally expected is that all the generic SII IRQs are
supplied as the separate signals meanwhile the rest of the SII signals
are combined in an additional line named like "app".

In your case we find an intermix of the SII generic IRQs and some SII
signals (though some of the names listed in your descriptions don't
match to what is defined in the DW PCIe HW manual). So what you said
in v1:

On Thu, Jul 13, 2023 at 7:47PM +0200, Sebastian Reichel wrote:
> I suppose "sys", "pmc", "msg" and "err" all fit for "app", since
> they are vendor specific with the extra layer? But obviously I
> cannot specify "app" more than once."

is mainly correct. For instance, the most of the generic SII interrupt
signals are combined in your "sys" IRQ, like "hp", "bw_au", "bw_mg",
"dma", "l_eq"; your "pmc" and "msg" IRQs are a set of the SII signals
not listed in the "SII Interrupt Signals" list; the "err" IRQ has the
"aer" generic SII Interrupt, but the rest of the signals are common SII
signals.

I am not fully certain of what to do in this case. Some possible options:

1. Keep the names defined as is, add them to the list of generic IRQ
names, describe them as "Combined IRQ signals" but with no specific
signals listed and with some generic meaningful description.
Alternatively create a separate sub-schema in the generic
"interrupt-names" property constraints in the same way as it's done
for the "vendor-specific IRQ names" and do the same with the names
descriptions. In anyway move your detailed descriptions to the
Rockchip DW PCIe DT-schema. In this case we imply that your names
could be re-used for some other device bindings.

2. Keep the names defined as is, add them to the list of
"vendor-specific IRQ names" sub-schema in the "interrupt-names"
property, describe each of them as "Combined IRQ signals" but with no
specific signals listed and with some generic meaningful description.
Move your detailed descriptions to the Rockchip DW PCIe DT-schema.

3. Add "app_" prefix to all your IRQs (except "legacy") and convert
the generic "app" IRQ name constraint to accepting a pattern like
'^app(_.*)?$' or similar. Move your detailed descriptions to the
Rockchip DW PCIe DT-schema.

4. Add Rockchip-specific prefix to the names (except "legacy"), add
all of them (for instance as a pattern-like schema) to the
vendor-specific IRQ names part of the "interrupt-names" items list
with a description referring to the Rockchip DT-bindings. Move your
detailed descriptions to the Rockchip DW PCIe DT-schema.

Doubtfully the categorization chosen by the Rockchip HW designers is
fully universal so the names could be utilized for other devices. Thus
IMO the options 2-4 might be more preferable over 1.

In anyway the detailed descriptions with the listed lines should be
in the Rockchip DW PCIe DT-bindings since they are definitely
vendor-specific.

Regarding the "legacy" name used as a combined "int(a|b|c|d)" IRQ.
Alas we can't change it. So it's either option 1 or 2. 

What do you think? Rob, Krzysztof, any better idea?

> +

>      allOf:
>        - contains:
> -          const: msi
> +          enum:
> +            - msi
> +            - msg

Based on the above the "msg" interrupt doesn't get to be required.
Rob, is it possible to have a constraint which would require either
the "msi" IRQ name or the "msi-map" DT-property or both?

-Serge(y)

>  
>  additionalProperties: true
>  
> -- 
> 2.40.1
>