Message-ID: <20230713164753.3dmxxejcr7litocl@mercury.elektranox.org>
Date:   Thu, 13 Jul 2023 18:47:53 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        linux-pci@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Simon Xue <xxm@...k-chips.com>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        kernel@...labora.com
Subject: Re: [PATCH v1 1/4] dt-bindings: PCI: dwc: rockchip: Fix
 interrupt-names issue

Hi,

Sorry for the delayed response.

On Tue, Jun 27, 2023 at 03:27:33PM +0300, Serge Semin wrote:
> 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@...labora.com>
> > ---
> >  .../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.)

All of these are combined interrupts and not simple aliases.
Otherwise I would just have changed the name for RK3588. Rockchip
has a two level interrupt system. I re-checked carefully and as far
as I can tell all interrupts currently defined in the binding have a
specific meaning. This is not the case for the interrupts from
RK3588. I will send a new version in a jiffy, which describes all
the sub-IRQs available beneath the newly described ones. I don't have
the Synopsys datasheet, so I will stick to the names used by Rockchip.

> > +        - 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

In case my explanation from above was not clear: All the other
interrupts follow the same style as this one.

> > +        - 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.

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.

> > +        - 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

pme should be 'msg -> pm_pme_int':

Interrupt indicates that the controller received a PM_PME message.

> > +        - description: Combined Message Received interrupt signal.
> > +          const: msg
> 
> ditto but with respect to the "msi" name.

MSI is handled via GIC-ITS using this DT property:

msi-map = <0x3000 &its0 0x3000 0x1000>;

> > +        - description: Combined Error interrupt signal.
> > +          const: err
> 
> ditto but with respect to the "sft_*" name.

I really meant it, when I wrote "Combined". Appart from
(un)correctable errors this also reports e.g. timeouts.

> > +
> >      allOf:
> >        - contains:
> > -          const: msi
> > +          enum:
> > +            - msi
> > +            - msg
> 
> * Please see my suggestion about converting the DTS file instead.

My understanding is, that "msi" and "msg" are not the same. MSI is
an interrupt message from a peripheral device, but "msg" is a
combined interrupt for all kind of messages.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists