| Message ID | 20220427112528.4097815-2-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: convert various Arm Ltd. IP to DT schema | expand |
On 2022-04-27 12:25, Andre Przywara wrote: > The Page Request Interface (PRI) is an optional PCIe feature. As such, a > SMMU would not need to handle it if the PCIe host bridge or the SMMU > itself do not implement it. Also an SMMU could be connected to a platform > device, without any PRI functionality whatsoever. > In all cases there would be no SMMU PRI queue interrupt to be wired up > to an interrupt controller. > > Relax the binding to allow specifying three interrupts, omitting the PRI > IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we > would need to sacrifice the command queue sync interrupt as well, which > might not be desired. > The Linux driver does not care about any order at all, just picks IRQs > based on their names, and treats all (wired) IRQs as optional. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > index e87bfbcc69135..6b3111f1f06ce 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > @@ -37,12 +37,23 @@ properties: > hardware supports just a single, combined interrupt line. > If provided, then the combined interrupt will be used in preference to > any others. > - - minItems: 2 > + - minItems: 1 > items: > - - const: eventq # Event Queue not empty > - - const: gerror # Global Error activated > - - const: priq # PRI Queue not empty > - - const: cmdq-sync # CMD_SYNC complete > + - enum: > + - eventq # Event Queue not empty > + - gerror # Global Error activated > + - cmdq-sync # CMD_SYNC complete > + - priq # PRI Queue not empty > + - enum: > + - gerror > + - cmdq-sync > + - priq > + - enum: > + - cmdq-sync > + - priq > + - enum: > + - cmdq-sync > + - priq Nit: the commit message doesn't mention this effective relaxation to allow "eventq, gerror, cmdq-sync, priq" as well - if there's a good reason for that it probably wants calling out, otherwise AFAICS the last item should stay as "- const: cmdq-sync" to preserve the existing canonical order for the 4-item case. Otherwise, Reviewed-by: Robin Murphy <robin.murphy@arm.com> Cheers, Robin. > > '#iommu-cells': > const: 1
On 27/04/2022 13:25, Andre Przywara wrote: > The Page Request Interface (PRI) is an optional PCIe feature. As such, a > SMMU would not need to handle it if the PCIe host bridge or the SMMU > itself do not implement it. Also an SMMU could be connected to a platform > device, without any PRI functionality whatsoever. > In all cases there would be no SMMU PRI queue interrupt to be wired up > to an interrupt controller. > > Relax the binding to allow specifying three interrupts, omitting the PRI > IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we > would need to sacrifice the command queue sync interrupt as well, which > might not be desired. > The Linux driver does not care about any order at all, just picks IRQs > based on their names, and treats all (wired) IRQs as optional. The last sentence is not a good explanation for the bindings. They are not about Linux and are used in other projects as well. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > index e87bfbcc69135..6b3111f1f06ce 100644 > --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > @@ -37,12 +37,23 @@ properties: > hardware supports just a single, combined interrupt line. > If provided, then the combined interrupt will be used in preference to > any others. > - - minItems: 2 > + - minItems: 1 > items: > - - const: eventq # Event Queue not empty > - - const: gerror # Global Error activated > - - const: priq # PRI Queue not empty > - - const: cmdq-sync # CMD_SYNC complete > + - enum: > + - eventq # Event Queue not empty > + - gerror # Global Error activated > + - cmdq-sync # CMD_SYNC complete > + - priq # PRI Queue not empty > + - enum: > + - gerror > + - cmdq-sync > + - priq > + - enum: > + - cmdq-sync > + - priq > + - enum: > + - cmdq-sync > + - priq The order should be strict, so if you want the first interrupt optional, then: oneOf: - items: ... 4 items list - items ... 3 items list Best regards, Krzysztof
On 2022-04-28 07:56, Krzysztof Kozlowski wrote: > On 27/04/2022 13:25, Andre Przywara wrote: >> The Page Request Interface (PRI) is an optional PCIe feature. As such, a >> SMMU would not need to handle it if the PCIe host bridge or the SMMU >> itself do not implement it. Also an SMMU could be connected to a platform >> device, without any PRI functionality whatsoever. >> In all cases there would be no SMMU PRI queue interrupt to be wired up >> to an interrupt controller. >> >> Relax the binding to allow specifying three interrupts, omitting the PRI >> IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we >> would need to sacrifice the command queue sync interrupt as well, which >> might not be desired. >> The Linux driver does not care about any order at all, just picks IRQs >> based on their names, and treats all (wired) IRQs as optional. > > The last sentence is not a good explanation for the bindings. They are > not about Linux and are used in other projects as well. > >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >> index e87bfbcc69135..6b3111f1f06ce 100644 >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >> @@ -37,12 +37,23 @@ properties: >> hardware supports just a single, combined interrupt line. >> If provided, then the combined interrupt will be used in preference to >> any others. >> - - minItems: 2 >> + - minItems: 1 >> items: >> - - const: eventq # Event Queue not empty >> - - const: gerror # Global Error activated >> - - const: priq # PRI Queue not empty >> - - const: cmdq-sync # CMD_SYNC complete >> + - enum: >> + - eventq # Event Queue not empty >> + - gerror # Global Error activated >> + - cmdq-sync # CMD_SYNC complete >> + - priq # PRI Queue not empty >> + - enum: >> + - gerror >> + - cmdq-sync >> + - priq >> + - enum: >> + - cmdq-sync >> + - priq >> + - enum: >> + - cmdq-sync >> + - priq > > The order should be strict, so if you want the first interrupt optional, > then: > oneOf: > - items: > ... 4 items list > - items > ... 3 items list All 4 interrupts are optional, though, since any of them could potentially use an MSI instead. Do we really want to list out all 15 combinations? Robin.
On 28/04/2022 11:23, Robin Murphy wrote: > On 2022-04-28 07:56, Krzysztof Kozlowski wrote: >> On 27/04/2022 13:25, Andre Przywara wrote: >>> The Page Request Interface (PRI) is an optional PCIe feature. As such, a >>> SMMU would not need to handle it if the PCIe host bridge or the SMMU >>> itself do not implement it. Also an SMMU could be connected to a platform >>> device, without any PRI functionality whatsoever. >>> In all cases there would be no SMMU PRI queue interrupt to be wired up >>> to an interrupt controller. >>> >>> Relax the binding to allow specifying three interrupts, omitting the PRI >>> IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we >>> would need to sacrifice the command queue sync interrupt as well, which >>> might not be desired. >>> The Linux driver does not care about any order at all, just picks IRQs >>> based on their names, and treats all (wired) IRQs as optional. >> >> The last sentence is not a good explanation for the bindings. They are >> not about Linux and are used in other projects as well. >> >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- >>> 1 file changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >>> index e87bfbcc69135..6b3111f1f06ce 100644 >>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml >>> @@ -37,12 +37,23 @@ properties: >>> hardware supports just a single, combined interrupt line. >>> If provided, then the combined interrupt will be used in preference to >>> any others. >>> - - minItems: 2 >>> + - minItems: 1 >>> items: >>> - - const: eventq # Event Queue not empty >>> - - const: gerror # Global Error activated >>> - - const: priq # PRI Queue not empty >>> - - const: cmdq-sync # CMD_SYNC complete >>> + - enum: >>> + - eventq # Event Queue not empty >>> + - gerror # Global Error activated >>> + - cmdq-sync # CMD_SYNC complete >>> + - priq # PRI Queue not empty >>> + - enum: >>> + - gerror >>> + - cmdq-sync >>> + - priq >>> + - enum: >>> + - cmdq-sync >>> + - priq >>> + - enum: >>> + - cmdq-sync >>> + - priq >> >> The order should be strict, so if you want the first interrupt optional, >> then: >> oneOf: >> - items: >> ... 4 items list >> - items >> ... 3 items list > > All 4 interrupts are optional, though, since any of them could > potentially use an MSI instead. Do we really want to list out all 15 > combinations? Bah, I missed that part from commit msg. It's fine then. Best regards, Krzysztof
On Thu, 28 Apr 2022 08:56:38 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: Hi, > On 27/04/2022 13:25, Andre Przywara wrote: > > The Page Request Interface (PRI) is an optional PCIe feature. As such, a > > SMMU would not need to handle it if the PCIe host bridge or the SMMU > > itself do not implement it. Also an SMMU could be connected to a platform > > device, without any PRI functionality whatsoever. > > In all cases there would be no SMMU PRI queue interrupt to be wired up > > to an interrupt controller. > > > > Relax the binding to allow specifying three interrupts, omitting the PRI > > IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we > > would need to sacrifice the command queue sync interrupt as well, which > > might not be desired. > > The Linux driver does not care about any order at all, just picks IRQs > > based on their names, and treats all (wired) IRQs as optional. > > The last sentence is not a good explanation for the bindings. They are > not about Linux and are used in other projects as well. It was not meant as an explanation, but just as an assurance that we can *change* the binding. At the moment the order is strict, so binding compliant DT consumers could just read the first, second, third, and fourth interrupt, without caring about the names. If we now allow a different order, this would break those users. I couldn't find any user of arm,smmu-v3 in FreeBSD, OpenBSD, U-Boot, or Zephyr, hence my mentioning of Linux being fine, so it's safe to relax the strict ordering requirement. If someone knows about other DT consumers that need attention, I am all ears. Cheers, Andre > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > > index e87bfbcc69135..6b3111f1f06ce 100644 > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml > > @@ -37,12 +37,23 @@ properties: > > hardware supports just a single, combined interrupt line. > > If provided, then the combined interrupt will be used in preference to > > any others. > > - - minItems: 2 > > + - minItems: 1 > > items: > > - - const: eventq # Event Queue not empty > > - - const: gerror # Global Error activated > > - - const: priq # PRI Queue not empty > > - - const: cmdq-sync # CMD_SYNC complete > > + - enum: > > + - eventq # Event Queue not empty > > + - gerror # Global Error activated > > + - cmdq-sync # CMD_SYNC complete > > + - priq # PRI Queue not empty > > + - enum: > > + - gerror > > + - cmdq-sync > > + - priq > > + - enum: > > + - cmdq-sync > > + - priq > > + - enum: > > + - cmdq-sync > > + - priq > > The order should be strict, so if you want the first interrupt optional, > then: > oneOf: > - items: > ... 4 items list > - items > ... 3 items list > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml index e87bfbcc69135..6b3111f1f06ce 100644 --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.yaml @@ -37,12 +37,23 @@ properties: hardware supports just a single, combined interrupt line. If provided, then the combined interrupt will be used in preference to any others. - - minItems: 2 + - minItems: 1 items: - - const: eventq # Event Queue not empty - - const: gerror # Global Error activated - - const: priq # PRI Queue not empty - - const: cmdq-sync # CMD_SYNC complete + - enum: + - eventq # Event Queue not empty + - gerror # Global Error activated + - cmdq-sync # CMD_SYNC complete + - priq # PRI Queue not empty + - enum: + - gerror + - cmdq-sync + - priq + - enum: + - cmdq-sync + - priq + - enum: + - cmdq-sync + - priq '#iommu-cells': const: 1
The Page Request Interface (PRI) is an optional PCIe feature. As such, a SMMU would not need to handle it if the PCIe host bridge or the SMMU itself do not implement it. Also an SMMU could be connected to a platform device, without any PRI functionality whatsoever. In all cases there would be no SMMU PRI queue interrupt to be wired up to an interrupt controller. Relax the binding to allow specifying three interrupts, omitting the PRI IRQ. At the moment, with the "eventq,gerror,priq,cmdq-sync" order, we would need to sacrifice the command queue sync interrupt as well, which might not be desired. The Linux driver does not care about any order at all, just picks IRQs based on their names, and treats all (wired) IRQs as optional. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- .../bindings/iommu/arm,smmu-v3.yaml | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)