diff mbox series

[09/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus

Message ID 20200521130008.8266-10-lorenzo.pieralisi@arm.com (mailing list archive)
State Not Applicable, archived
Delegated to: Lorenzo Pieralisi
Headers show
Series ACPI/OF: Upgrade MSI/IOMMU ID mapping APIs | expand

Commit Message

Lorenzo Pieralisi May 21, 2020, 1 p.m. UTC
From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

The existing bindings cannot be used to specify the relationship
between fsl-mc devices and GIC ITSes.

Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
msi-map property.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: Rob Herring <robh+dt@kernel.org>
---
 .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Rob Herring May 21, 2020, 11:10 p.m. UTC | #1
On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>
> The existing bindings cannot be used to specify the relationship
> between fsl-mc devices and GIC ITSes.
>
> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
> msi-map property.
>
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> ---
>  .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> index 9134e9bcca56..b0813b2d0493 100644
> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit value called an ICID
>  the requester.
>
>  The generic 'iommus' property is insufficient to describe the relationship
> -between ICIDs and IOMMUs, so an iommu-map property is used to define
> -the set of possible ICIDs under a root DPRC and how they map to
> -an IOMMU.
> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
> +to define the set of possible ICIDs under a root DPRC and how they map to
> +an IOMMU and a GIC ITS respectively.
>
>  For generic IOMMU bindings, see
>  Documentation/devicetree/bindings/iommu/iommu.txt.
> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>  For arm-smmu binding, see:
>  Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>
> +For GICv3 and GIC ITS bindings, see:
> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
> +
>  Required properties:
>
>      - compatible
> @@ -119,6 +122,15 @@ Optional properties:
>    associated with the listed IOMMU, with the iommu-specifier
>    (i - icid-base + iommu-base).
>
> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
> +  data.
> +
> +  The property is an arbitrary number of tuples of
> +  (icid-base,iommu,iommu-base,length).

I'm confused because the example has GIC ITS phandle, not an IOMMU.

What is an iommu-base?

> +
> +  Any ICID in the interval [icid-base, icid-base + length) is
> +  associated with the listed GIC ITS, with the iommu-specifier
> +  (i - icid-base + iommu-base).
>  Example:
>
>          smmu: iommu@5000000 {
> @@ -128,6 +140,16 @@ Example:
>                 ...
>          };
>
> +       gic: interrupt-controller@6000000 {
> +               compatible = "arm,gic-v3";
> +               ...
> +               its: gic-its@6020000 {
> +                       compatible = "arm,gic-v3-its";
> +                       msi-controller;
> +                       ...
> +               };
> +       };
> +
>          fsl_mc: fsl-mc@80c000000 {
>                  compatible = "fsl,qoriq-mc";
>                  reg = <0x00000008 0x0c000000 0 0x40>,    /* MC portal base */
> @@ -135,6 +157,8 @@ Example:
>                  msi-parent = <&its>;
>                  /* define map for ICIDs 23-64 */
>                  iommu-map = <23 &smmu 23 41>;
> +                /* define msi map for ICIDs 23-64 */
> +                msi-map = <23 &its 23 41>;

Seeing 23 twice is odd. The numbers to the right of 'its' should be an
ITS number space.

>                  #address-cells = <3>;
>                  #size-cells = <1>;
>
> --
> 2.26.1
>
Robin Murphy May 22, 2020, 9:42 a.m. UTC | #2
On 2020-05-22 00:10, Rob Herring wrote:
> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>>
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> The existing bindings cannot be used to specify the relationship
>> between fsl-mc devices and GIC ITSes.
>>
>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>> msi-map property.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>>   .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
>>   1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> index 9134e9bcca56..b0813b2d0493 100644
>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit value called an ICID
>>   the requester.
>>
>>   The generic 'iommus' property is insufficient to describe the relationship
>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>> -the set of possible ICIDs under a root DPRC and how they map to
>> -an IOMMU.
>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
>> +to define the set of possible ICIDs under a root DPRC and how they map to
>> +an IOMMU and a GIC ITS respectively.
>>
>>   For generic IOMMU bindings, see
>>   Documentation/devicetree/bindings/iommu/iommu.txt.
>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>>   For arm-smmu binding, see:
>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>
>> +For GICv3 and GIC ITS bindings, see:
>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
>> +
>>   Required properties:
>>
>>       - compatible
>> @@ -119,6 +122,15 @@ Optional properties:
>>     associated with the listed IOMMU, with the iommu-specifier
>>     (i - icid-base + iommu-base).
>>
>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>> +  data.
>> +
>> +  The property is an arbitrary number of tuples of
>> +  (icid-base,iommu,iommu-base,length).
> 
> I'm confused because the example has GIC ITS phandle, not an IOMMU.
> 
> What is an iommu-base?

Right, I was already halfway through writing a reply to say that all the 
copy-pasted "iommu" references here should be using the terminology from 
the pci-msi.txt binding instead.

>> +
>> +  Any ICID in the interval [icid-base, icid-base + length) is
>> +  associated with the listed GIC ITS, with the iommu-specifier
>> +  (i - icid-base + iommu-base).
>>   Example:
>>
>>           smmu: iommu@5000000 {
>> @@ -128,6 +140,16 @@ Example:
>>                  ...
>>           };
>>
>> +       gic: interrupt-controller@6000000 {
>> +               compatible = "arm,gic-v3";
>> +               ...
>> +               its: gic-its@6020000 {
>> +                       compatible = "arm,gic-v3-its";
>> +                       msi-controller;
>> +                       ...
>> +               };
>> +       };
>> +
>>           fsl_mc: fsl-mc@80c000000 {
>>                   compatible = "fsl,qoriq-mc";
>>                   reg = <0x00000008 0x0c000000 0 0x40>,    /* MC portal base */
>> @@ -135,6 +157,8 @@ Example:
>>                   msi-parent = <&its>;

Side note: is it right to keep msi-parent here? It rather implies that 
the MC itself has a 'native' Device ID rather than an ICID, which I 
believe is not strictly true. Plus it's extra-confusing that it doesn't 
specify an ID either way, since that makes it look like the legacy PCI 
case that gets treated implicitly as an identity msi-map, which makes no 
sense at all to combine with an actual msi-map.

>>                   /* define map for ICIDs 23-64 */
>>                   iommu-map = <23 &smmu 23 41>;
>> +                /* define msi map for ICIDs 23-64 */
>> +                msi-map = <23 &its 23 41>;
> 
> Seeing 23 twice is odd. The numbers to the right of 'its' should be an
> ITS number space.

On about 99% of systems the values in the SMMU Stream ID and ITS Device 
ID spaces are going to be the same. Nobody's going to bother carrying 
*two* sets of sideband data across the interconnect if they don't have to ;)

Robin.

>>                   #address-cells = <3>;
>>                   #size-cells = <1>;
>>
>> --
>> 2.26.1
>>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
Diana Madalina Craciun May 22, 2020, 9:57 a.m. UTC | #3
On 5/22/2020 12:42 PM, Robin Murphy wrote:
> On 2020-05-22 00:10, Rob Herring wrote:
>> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>>
>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>
>>> The existing bindings cannot be used to specify the relationship
>>> between fsl-mc devices and GIC ITSes.
>>>
>>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>>> msi-map property.
>>>
>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> ---
>>>   .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 
>>> +++++++++++++++++--
>>>   1 file changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt 
>>> b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> index 9134e9bcca56..b0813b2d0493 100644
>>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit 
>>> value called an ICID
>>>   the requester.
>>>
>>>   The generic 'iommus' property is insufficient to describe the 
>>> relationship
>>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>>> -the set of possible ICIDs under a root DPRC and how they map to
>>> -an IOMMU.
>>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties 
>>> are used
>>> +to define the set of possible ICIDs under a root DPRC and how they 
>>> map to
>>> +an IOMMU and a GIC ITS respectively.
>>>
>>>   For generic IOMMU bindings, see
>>>   Documentation/devicetree/bindings/iommu/iommu.txt.
>>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>>>   For arm-smmu binding, see:
>>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>>
>>> +For GICv3 and GIC ITS bindings, see:
>>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml. 
>>>
>>> +
>>>   Required properties:
>>>
>>>       - compatible
>>> @@ -119,6 +122,15 @@ Optional properties:
>>>     associated with the listed IOMMU, with the iommu-specifier
>>>     (i - icid-base + iommu-base).
>>>
>>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>>> +  data.
>>> +
>>> +  The property is an arbitrary number of tuples of
>>> +  (icid-base,iommu,iommu-base,length).
>>
>> I'm confused because the example has GIC ITS phandle, not an IOMMU.
>>
>> What is an iommu-base?
>
> Right, I was already halfway through writing a reply to say that all 
> the copy-pasted "iommu" references here should be using the 
> terminology from the pci-msi.txt binding instead.

Right, will change it.

>
>>> +
>>> +  Any ICID in the interval [icid-base, icid-base + length) is
>>> +  associated with the listed GIC ITS, with the iommu-specifier
>>> +  (i - icid-base + iommu-base).
>>>   Example:
>>>
>>>           smmu: iommu@5000000 {
>>> @@ -128,6 +140,16 @@ Example:
>>>                  ...
>>>           };
>>>
>>> +       gic: interrupt-controller@6000000 {
>>> +               compatible = "arm,gic-v3";
>>> +               ...
>>> +               its: gic-its@6020000 {
>>> +                       compatible = "arm,gic-v3-its";
>>> +                       msi-controller;
>>> +                       ...
>>> +               };
>>> +       };
>>> +
>>>           fsl_mc: fsl-mc@80c000000 {
>>>                   compatible = "fsl,qoriq-mc";
>>>                   reg = <0x00000008 0x0c000000 0 0x40>,    /* MC 
>>> portal base */
>>> @@ -135,6 +157,8 @@ Example:
>>>                   msi-parent = <&its>;
>
> Side note: is it right to keep msi-parent here? It rather implies that 
> the MC itself has a 'native' Device ID rather than an ICID, which I 
> believe is not strictly true. Plus it's extra-confusing that it 
> doesn't specify an ID either way, since that makes it look like the 
> legacy PCI case that gets treated implicitly as an identity msi-map, 
> which makes no sense at all to combine with an actual msi-map.

Before adding msi-map, the fsl-mc code assumed that ICID and streamID 
are equal and used msi-parent just to get the reference to the ITS node. 
Removing msi-parent will break the backward compatibility of the already 
existing systems. Maybe we should mention that this is legacy and not to 
be used for newer device trees.


>
>>>                   /* define map for ICIDs 23-64 */
>>>                   iommu-map = <23 &smmu 23 41>;
>>> +                /* define msi map for ICIDs 23-64 */
>>> +                msi-map = <23 &its 23 41>;
>>
>> Seeing 23 twice is odd. The numbers to the right of 'its' should be an
>> ITS number space.
>
> On about 99% of systems the values in the SMMU Stream ID and ITS 
> Device ID spaces are going to be the same. Nobody's going to bother 
> carrying *two* sets of sideband data across the interconnect if they 
> don't have to ;)
>
> Robin.

Diana


>
>>>                   #address-cells = <3>;
>>>                   #size-cells = <1>;
>>>
>>> -- 
>>> 2.26.1
>>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
Rob Herring May 22, 2020, 2:02 p.m. UTC | #4
On Fri, May 22, 2020 at 3:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-05-22 00:10, Rob Herring wrote:
> > On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> >>
> >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >>
> >> The existing bindings cannot be used to specify the relationship
> >> between fsl-mc devices and GIC ITSes.
> >>
> >> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
> >> msi-map property.
> >>
> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> ---
> >>   .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
> >>   1 file changed, 27 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >> index 9134e9bcca56..b0813b2d0493 100644
> >> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit value called an ICID
> >>   the requester.
> >>
> >>   The generic 'iommus' property is insufficient to describe the relationship
> >> -between ICIDs and IOMMUs, so an iommu-map property is used to define
> >> -the set of possible ICIDs under a root DPRC and how they map to
> >> -an IOMMU.
> >> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
> >> +to define the set of possible ICIDs under a root DPRC and how they map to
> >> +an IOMMU and a GIC ITS respectively.
> >>
> >>   For generic IOMMU bindings, see
> >>   Documentation/devicetree/bindings/iommu/iommu.txt.
> >> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
> >>   For arm-smmu binding, see:
> >>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
> >>
> >> +For GICv3 and GIC ITS bindings, see:
> >> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
> >> +
> >>   Required properties:
> >>
> >>       - compatible
> >> @@ -119,6 +122,15 @@ Optional properties:
> >>     associated with the listed IOMMU, with the iommu-specifier
> >>     (i - icid-base + iommu-base).
> >>
> >> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
> >> +  data.
> >> +
> >> +  The property is an arbitrary number of tuples of
> >> +  (icid-base,iommu,iommu-base,length).
> >
> > I'm confused because the example has GIC ITS phandle, not an IOMMU.
> >
> > What is an iommu-base?
>
> Right, I was already halfway through writing a reply to say that all the
> copy-pasted "iommu" references here should be using the terminology from
> the pci-msi.txt binding instead.
>
> >> +
> >> +  Any ICID in the interval [icid-base, icid-base + length) is
> >> +  associated with the listed GIC ITS, with the iommu-specifier
> >> +  (i - icid-base + iommu-base).
> >>   Example:
> >>
> >>           smmu: iommu@5000000 {
> >> @@ -128,6 +140,16 @@ Example:
> >>                  ...
> >>           };
> >>
> >> +       gic: interrupt-controller@6000000 {
> >> +               compatible = "arm,gic-v3";
> >> +               ...
> >> +               its: gic-its@6020000 {
> >> +                       compatible = "arm,gic-v3-its";
> >> +                       msi-controller;
> >> +                       ...
> >> +               };
> >> +       };
> >> +
> >>           fsl_mc: fsl-mc@80c000000 {
> >>                   compatible = "fsl,qoriq-mc";
> >>                   reg = <0x00000008 0x0c000000 0 0x40>,    /* MC portal base */
> >> @@ -135,6 +157,8 @@ Example:
> >>                   msi-parent = <&its>;
>
> Side note: is it right to keep msi-parent here? It rather implies that
> the MC itself has a 'native' Device ID rather than an ICID, which I
> believe is not strictly true. Plus it's extra-confusing that it doesn't
> specify an ID either way, since that makes it look like the legacy PCI
> case that gets treated implicitly as an identity msi-map, which makes no
> sense at all to combine with an actual msi-map.

No, it doesn't make sense from a binding perspective.

>
> >>                   /* define map for ICIDs 23-64 */
> >>                   iommu-map = <23 &smmu 23 41>;
> >> +                /* define msi map for ICIDs 23-64 */
> >> +                msi-map = <23 &its 23 41>;
> >
> > Seeing 23 twice is odd. The numbers to the right of 'its' should be an
> > ITS number space.
>
> On about 99% of systems the values in the SMMU Stream ID and ITS Device
> ID spaces are going to be the same. Nobody's going to bother carrying
> *two* sets of sideband data across the interconnect if they don't have to ;)

I'm referring to the 23 on the left and right, not between the msi and
iommu. If the left and right are the same, then what are we remapping
exactly?

Rob
Rob Herring May 22, 2020, 2:08 p.m. UTC | #5
On Fri, May 22, 2020 at 3:57 AM Diana Craciun OSS
<diana.craciun@oss.nxp.com> wrote:
>
> On 5/22/2020 12:42 PM, Robin Murphy wrote:
> > On 2020-05-22 00:10, Rob Herring wrote:
> >> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> >> <lorenzo.pieralisi@arm.com> wrote:
> >>>
> >>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >>>
> >>> The existing bindings cannot be used to specify the relationship
> >>> between fsl-mc devices and GIC ITSes.
> >>>
> >>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
> >>> msi-map property.
> >>>
> >>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >>> ---
> >>>   .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30
> >>> +++++++++++++++++--
> >>>   1 file changed, 27 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >>> b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >>> index 9134e9bcca56..b0813b2d0493 100644
> >>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
> >>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit
> >>> value called an ICID
> >>>   the requester.
> >>>
> >>>   The generic 'iommus' property is insufficient to describe the
> >>> relationship
> >>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
> >>> -the set of possible ICIDs under a root DPRC and how they map to
> >>> -an IOMMU.
> >>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties
> >>> are used
> >>> +to define the set of possible ICIDs under a root DPRC and how they
> >>> map to
> >>> +an IOMMU and a GIC ITS respectively.
> >>>
> >>>   For generic IOMMU bindings, see
> >>>   Documentation/devicetree/bindings/iommu/iommu.txt.
> >>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
> >>>   For arm-smmu binding, see:
> >>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
> >>>
> >>> +For GICv3 and GIC ITS bindings, see:
> >>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
> >>>
> >>> +
> >>>   Required properties:
> >>>
> >>>       - compatible
> >>> @@ -119,6 +122,15 @@ Optional properties:
> >>>     associated with the listed IOMMU, with the iommu-specifier
> >>>     (i - icid-base + iommu-base).
> >>>
> >>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
> >>> +  data.
> >>> +
> >>> +  The property is an arbitrary number of tuples of
> >>> +  (icid-base,iommu,iommu-base,length).
> >>
> >> I'm confused because the example has GIC ITS phandle, not an IOMMU.
> >>
> >> What is an iommu-base?
> >
> > Right, I was already halfway through writing a reply to say that all
> > the copy-pasted "iommu" references here should be using the
> > terminology from the pci-msi.txt binding instead.
>
> Right, will change it.
>
> >
> >>> +
> >>> +  Any ICID in the interval [icid-base, icid-base + length) is
> >>> +  associated with the listed GIC ITS, with the iommu-specifier
> >>> +  (i - icid-base + iommu-base).
> >>>   Example:
> >>>
> >>>           smmu: iommu@5000000 {
> >>> @@ -128,6 +140,16 @@ Example:
> >>>                  ...
> >>>           };
> >>>
> >>> +       gic: interrupt-controller@6000000 {
> >>> +               compatible = "arm,gic-v3";
> >>> +               ...
> >>> +               its: gic-its@6020000 {
> >>> +                       compatible = "arm,gic-v3-its";
> >>> +                       msi-controller;
> >>> +                       ...
> >>> +               };
> >>> +       };
> >>> +
> >>>           fsl_mc: fsl-mc@80c000000 {
> >>>                   compatible = "fsl,qoriq-mc";
> >>>                   reg = <0x00000008 0x0c000000 0 0x40>,    /* MC
> >>> portal base */
> >>> @@ -135,6 +157,8 @@ Example:
> >>>                   msi-parent = <&its>;
> >
> > Side note: is it right to keep msi-parent here? It rather implies that
> > the MC itself has a 'native' Device ID rather than an ICID, which I
> > believe is not strictly true. Plus it's extra-confusing that it
> > doesn't specify an ID either way, since that makes it look like the
> > legacy PCI case that gets treated implicitly as an identity msi-map,
> > which makes no sense at all to combine with an actual msi-map.
>
> Before adding msi-map, the fsl-mc code assumed that ICID and streamID
> are equal and used msi-parent just to get the reference to the ITS node.
> Removing msi-parent will break the backward compatibility of the already
> existing systems. Maybe we should mention that this is legacy and not to
> be used for newer device trees.

If ids are 1:1, then the DT should use msi-parent. If there is
remapping, then use msi-map. A given system should use one or the
other. I suppose if some ids are 1:1 and the msi-map was added to add
additional support for ids not 1:1, then you could end up with both.
That's fine in dts files, but examples should reflect the 'right' way.

Rob
Robin Murphy May 22, 2020, 2:34 p.m. UTC | #6
On 2020-05-22 15:08, Rob Herring wrote:
> On Fri, May 22, 2020 at 3:57 AM Diana Craciun OSS
> <diana.craciun@oss.nxp.com> wrote:
>>
>> On 5/22/2020 12:42 PM, Robin Murphy wrote:
>>> On 2020-05-22 00:10, Rob Herring wrote:
>>>> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>
>>>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>>>
>>>>> The existing bindings cannot be used to specify the relationship
>>>>> between fsl-mc devices and GIC ITSes.
>>>>>
>>>>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>>>>> msi-map property.
>>>>>
>>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> ---
>>>>>    .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30
>>>>> +++++++++++++++++--
>>>>>    1 file changed, 27 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>>> b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>>> index 9134e9bcca56..b0813b2d0493 100644
>>>>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit
>>>>> value called an ICID
>>>>>    the requester.
>>>>>
>>>>>    The generic 'iommus' property is insufficient to describe the
>>>>> relationship
>>>>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>>>>> -the set of possible ICIDs under a root DPRC and how they map to
>>>>> -an IOMMU.
>>>>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties
>>>>> are used
>>>>> +to define the set of possible ICIDs under a root DPRC and how they
>>>>> map to
>>>>> +an IOMMU and a GIC ITS respectively.
>>>>>
>>>>>    For generic IOMMU bindings, see
>>>>>    Documentation/devicetree/bindings/iommu/iommu.txt.
>>>>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>>>>>    For arm-smmu binding, see:
>>>>>    Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>>>>
>>>>> +For GICv3 and GIC ITS bindings, see:
>>>>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
>>>>>
>>>>> +
>>>>>    Required properties:
>>>>>
>>>>>        - compatible
>>>>> @@ -119,6 +122,15 @@ Optional properties:
>>>>>      associated with the listed IOMMU, with the iommu-specifier
>>>>>      (i - icid-base + iommu-base).
>>>>>
>>>>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>>>>> +  data.
>>>>> +
>>>>> +  The property is an arbitrary number of tuples of
>>>>> +  (icid-base,iommu,iommu-base,length).
>>>>
>>>> I'm confused because the example has GIC ITS phandle, not an IOMMU.
>>>>
>>>> What is an iommu-base?
>>>
>>> Right, I was already halfway through writing a reply to say that all
>>> the copy-pasted "iommu" references here should be using the
>>> terminology from the pci-msi.txt binding instead.
>>
>> Right, will change it.
>>
>>>
>>>>> +
>>>>> +  Any ICID in the interval [icid-base, icid-base + length) is
>>>>> +  associated with the listed GIC ITS, with the iommu-specifier
>>>>> +  (i - icid-base + iommu-base).
>>>>>    Example:
>>>>>
>>>>>            smmu: iommu@5000000 {
>>>>> @@ -128,6 +140,16 @@ Example:
>>>>>                   ...
>>>>>            };
>>>>>
>>>>> +       gic: interrupt-controller@6000000 {
>>>>> +               compatible = "arm,gic-v3";
>>>>> +               ...
>>>>> +               its: gic-its@6020000 {
>>>>> +                       compatible = "arm,gic-v3-its";
>>>>> +                       msi-controller;
>>>>> +                       ...
>>>>> +               };
>>>>> +       };
>>>>> +
>>>>>            fsl_mc: fsl-mc@80c000000 {
>>>>>                    compatible = "fsl,qoriq-mc";
>>>>>                    reg = <0x00000008 0x0c000000 0 0x40>,    /* MC
>>>>> portal base */
>>>>> @@ -135,6 +157,8 @@ Example:
>>>>>                    msi-parent = <&its>;
>>>
>>> Side note: is it right to keep msi-parent here? It rather implies that
>>> the MC itself has a 'native' Device ID rather than an ICID, which I
>>> believe is not strictly true. Plus it's extra-confusing that it
>>> doesn't specify an ID either way, since that makes it look like the
>>> legacy PCI case that gets treated implicitly as an identity msi-map,
>>> which makes no sense at all to combine with an actual msi-map.
>>
>> Before adding msi-map, the fsl-mc code assumed that ICID and streamID
>> are equal and used msi-parent just to get the reference to the ITS node.
>> Removing msi-parent will break the backward compatibility of the already
>> existing systems. Maybe we should mention that this is legacy and not to
>> be used for newer device trees.
> 
> If ids are 1:1, then the DT should use msi-parent. If there is
> remapping, then use msi-map. A given system should use one or the
> other. I suppose if some ids are 1:1 and the msi-map was added to add
> additional support for ids not 1:1, then you could end up with both.
> That's fine in dts files, but examples should reflect the 'right' way.

Is that defined anywhere? The generic MSI binding just has some weaselly 
wording about buses:

"When #msi-cells is non-zero, busses with an msi-parent will require 
additional properties to describe the relationship between devices on 
the bus and the set of MSIs they can potentially generate."

which appears at odds with its own definition of msi-parent as including 
an msi-specifier (or at best very unclear about what value that 
specifier should take in this case).

The PCI MSI binding goes even further and specifically reserves 
msi-parent for cases where there is no sideband data. As far as I'm 
aware, the fact that the ITS driver implements a bodge for the "empty 
msi-parent even though #msi-cells > 0" case is merely a compatibility 
thing for old DTs from before this was really thought through, not an 
officially-specified behaviour.

Robin.
Laurentiu Tudor May 22, 2020, 3:38 p.m. UTC | #7
On 5/22/2020 5:02 PM, Rob Herring wrote:
> On Fri, May 22, 2020 at 3:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-05-22 00:10, Rob Herring wrote:
>>> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>
>>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>>
>>>> The existing bindings cannot be used to specify the relationship
>>>> between fsl-mc devices and GIC ITSes.
>>>>
>>>> Add a generic binding for mapping fsl-mc devices to GIC ITSes, using
>>>> msi-map property.
>>>>
>>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> ---
>>>>   .../devicetree/bindings/misc/fsl,qoriq-mc.txt | 30 +++++++++++++++++--
>>>>   1 file changed, 27 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>> index 9134e9bcca56..b0813b2d0493 100644
>>>> --- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>> +++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>>>> @@ -18,9 +18,9 @@ same hardware "isolation context" and a 10-bit value called an ICID
>>>>   the requester.
>>>>
>>>>   The generic 'iommus' property is insufficient to describe the relationship
>>>> -between ICIDs and IOMMUs, so an iommu-map property is used to define
>>>> -the set of possible ICIDs under a root DPRC and how they map to
>>>> -an IOMMU.
>>>> +between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
>>>> +to define the set of possible ICIDs under a root DPRC and how they map to
>>>> +an IOMMU and a GIC ITS respectively.
>>>>
>>>>   For generic IOMMU bindings, see
>>>>   Documentation/devicetree/bindings/iommu/iommu.txt.
>>>> @@ -28,6 +28,9 @@ Documentation/devicetree/bindings/iommu/iommu.txt.
>>>>   For arm-smmu binding, see:
>>>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
>>>>
>>>> +For GICv3 and GIC ITS bindings, see:
>>>> +Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
>>>> +
>>>>   Required properties:
>>>>
>>>>       - compatible
>>>> @@ -119,6 +122,15 @@ Optional properties:
>>>>     associated with the listed IOMMU, with the iommu-specifier
>>>>     (i - icid-base + iommu-base).
>>>>
>>>> +- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
>>>> +  data.
>>>> +
>>>> +  The property is an arbitrary number of tuples of
>>>> +  (icid-base,iommu,iommu-base,length).
>>>
>>> I'm confused because the example has GIC ITS phandle, not an IOMMU.
>>>
>>> What is an iommu-base?
>>
>> Right, I was already halfway through writing a reply to say that all the
>> copy-pasted "iommu" references here should be using the terminology from
>> the pci-msi.txt binding instead.
>>
>>>> +
>>>> +  Any ICID in the interval [icid-base, icid-base + length) is
>>>> +  associated with the listed GIC ITS, with the iommu-specifier
>>>> +  (i - icid-base + iommu-base).
>>>>   Example:
>>>>
>>>>           smmu: iommu@5000000 {
>>>> @@ -128,6 +140,16 @@ Example:
>>>>                  ...
>>>>           };
>>>>
>>>> +       gic: interrupt-controller@6000000 {
>>>> +               compatible = "arm,gic-v3";
>>>> +               ...
>>>> +               its: gic-its@6020000 {
>>>> +                       compatible = "arm,gic-v3-its";
>>>> +                       msi-controller;
>>>> +                       ...
>>>> +               };
>>>> +       };
>>>> +
>>>>           fsl_mc: fsl-mc@80c000000 {
>>>>                   compatible = "fsl,qoriq-mc";
>>>>                   reg = <0x00000008 0x0c000000 0 0x40>,    /* MC portal base */
>>>> @@ -135,6 +157,8 @@ Example:
>>>>                   msi-parent = <&its>;
>>
>> Side note: is it right to keep msi-parent here? It rather implies that
>> the MC itself has a 'native' Device ID rather than an ICID, which I
>> believe is not strictly true. Plus it's extra-confusing that it doesn't
>> specify an ID either way, since that makes it look like the legacy PCI
>> case that gets treated implicitly as an identity msi-map, which makes no
>> sense at all to combine with an actual msi-map.
> 
> No, it doesn't make sense from a binding perspective.
> 
>>
>>>>                   /* define map for ICIDs 23-64 */
>>>>                   iommu-map = <23 &smmu 23 41>;
>>>> +                /* define msi map for ICIDs 23-64 */
>>>> +                msi-map = <23 &its 23 41>;
>>>
>>> Seeing 23 twice is odd. The numbers to the right of 'its' should be an
>>> ITS number space.
>>
>> On about 99% of systems the values in the SMMU Stream ID and ITS Device
>> ID spaces are going to be the same. Nobody's going to bother carrying
>> *two* sets of sideband data across the interconnect if they don't have to ;)
> 
> I'm referring to the 23 on the left and right, not between the msi and
> iommu. If the left and right are the same, then what are we remapping
> exactly?
> 

I also insisted a lot on keeping things simple and don't do any kind of
translation but Robin convinced me that this is not such a great idea.
The truth is that the hardware can be configured in such a way that the
assumption that icid -> streamid mapping is 1:1 no longer holds.
It just happens that we currently setup the hw to have 1:1 mappings.

P.S. No idea why, but somehow I got dropped from the thread. Weird.

---
Best Regards, Laurentiu
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
index 9134e9bcca56..b0813b2d0493 100644
--- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
+++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
@@ -18,9 +18,9 @@  same hardware "isolation context" and a 10-bit value called an ICID
 the requester.
 
 The generic 'iommus' property is insufficient to describe the relationship
-between ICIDs and IOMMUs, so an iommu-map property is used to define
-the set of possible ICIDs under a root DPRC and how they map to
-an IOMMU.
+between ICIDs and IOMMUs, so the iommu-map and msi-map properties are used
+to define the set of possible ICIDs under a root DPRC and how they map to
+an IOMMU and a GIC ITS respectively.
 
 For generic IOMMU bindings, see
 Documentation/devicetree/bindings/iommu/iommu.txt.
@@ -28,6 +28,9 @@  Documentation/devicetree/bindings/iommu/iommu.txt.
 For arm-smmu binding, see:
 Documentation/devicetree/bindings/iommu/arm,smmu.yaml.
 
+For GICv3 and GIC ITS bindings, see:
+Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml.
+
 Required properties:
 
     - compatible
@@ -119,6 +122,15 @@  Optional properties:
   associated with the listed IOMMU, with the iommu-specifier
   (i - icid-base + iommu-base).
 
+- msi-map: Maps an ICID to a GIC ITS and associated iommu-specifier
+  data.
+
+  The property is an arbitrary number of tuples of
+  (icid-base,iommu,iommu-base,length).
+
+  Any ICID in the interval [icid-base, icid-base + length) is
+  associated with the listed GIC ITS, with the iommu-specifier
+  (i - icid-base + iommu-base).
 Example:
 
         smmu: iommu@5000000 {
@@ -128,6 +140,16 @@  Example:
                ...
         };
 
+	gic: interrupt-controller@6000000 {
+		compatible = "arm,gic-v3";
+		...
+		its: gic-its@6020000 {
+			compatible = "arm,gic-v3-its";
+			msi-controller;
+			...
+		};
+	};
+
         fsl_mc: fsl-mc@80c000000 {
                 compatible = "fsl,qoriq-mc";
                 reg = <0x00000008 0x0c000000 0 0x40>,    /* MC portal base */
@@ -135,6 +157,8 @@  Example:
                 msi-parent = <&its>;
                 /* define map for ICIDs 23-64 */
                 iommu-map = <23 &smmu 23 41>;
+                /* define msi map for ICIDs 23-64 */
+                msi-map = <23 &its 23 41>;
                 #address-cells = <3>;
                 #size-cells = <1>;