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

[Serge Semin]

On Fri, Jun 16, 2023 at 07:00:19PM +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  | 15 ++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 24c88942e59e..98e45d2d8dfe 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -56,6 +56,17 @@ properties:
>        - const: pclk
>        - const: aux
>  
> +  interrupts:
> +    maxItems: 5
> +
> +  interrupt-names:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +
>    msi-map: true
>  
>    num-lanes: true
> @@ -98,6 +109,7 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>      bus {
>          #address-cells = <2>;
> @@ -117,6 +129,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..9f605eb297f5 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -193,9 +193,22 @@ properties:
>            oneOf:
>              - description: See native "app" IRQ for details
>                enum: [ intr ]

The IRQs below are either combined version of the already defined IRQs
or just the generic DW PCIe IRQs but named differently. Moreover I
don't see kernel using any of them except the "legacy" interrupt. What
about converting the dts files to using the already defined names instead
of extending the already over-diverged DT-bindings?
Rob, Krzysztof?

In anyway in order to prevent from defining the new DW PCIe bindings
compatible with your vendor-specific names please move the aliases to
being under the last entry of the "interrupt-names" items property.
(See the "intr" IRQ name for example or the way the vendor-specific
names are defined in the reg-names property.)

> +        - description: Combined Legacy A/B/C/D interrupt signal.
> +          const: legacy

This is a combined signal of "^int(a|b|c|d)$". So the entry
is supposed to look:
+              - description: See native "int*" IRQ for details
+                const: legacy

> +        - description: Combined System interrupt signal.
> +          const: sys

This seems like the "app" interrupt. So please either convert the dts
file to using the "app" name or move this to being defined in the same
entry as the "intr" name.

> +        - description: Combined Power Management interrupt signal.
> +          const: pmc

This is an alias to the already defined "pme" name. So either convert
the dts file to using "pme" or move this to being in the
vendor-specific list of the "interrupt-names" property:
+              - description: See native "pme" IRQ for details
+                const: pmc

> +        - description: Combined Message Received interrupt signal.
> +          const: msg

ditto but with respect to the "msi" name.

> +        - description: Combined Error interrupt signal.
> +          const: err

ditto but with respect to the "sft_*" name.

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

* Please see my suggestion about converting the DTS file instead.

-Serge(y)

>  
>  additionalProperties: true
>  
> -- 
> 2.39.2
>