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

From: Sebastian Reichel
Date: Thu Jul 13 2023 - 12:48:12 EST

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@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.)

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

Attachment: signature.asc
Description: PGP signature