Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Rob Herring <robh+dt@kernel.org>,
	Diana Craciun OSS <diana.craciun@oss.nxp.com>
Cc: devicetree@vger.kernel.org, Hanjun Guo <guohanjun@huawei.com>,
	Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
	PCI <linux-pci@vger.kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux IOMMU <iommu@lists.linux-foundation.org>,
	linux-acpi@vger.kernel.org,
	Makarand Pawagi <makarand.pawagi@nxp.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 09/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus
Date: Fri, 22 May 2020 15:34:27 +0100
Message-ID: <7f8d00ae-b225-a58d-8a11-e8c68edc877b@arm.com> (raw)
In-Reply-To: <CAL_JsqKf+cq9Nhs+M8ihC-Ls24YH-WEofW8H4kkFPWMhZw=unA@mail.gmail.com>

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.

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 12:59 [PATCH 00/12] ACPI/OF: Upgrade MSI/IOMMU ID mapping APIs Lorenzo Pieralisi
2020-05-21 12:59 ` [PATCH 01/12] ACPI/IORT: Make iort_match_node_callback walk the ACPI namespace for NC Lorenzo Pieralisi
2020-05-21 12:59 ` [PATCH 02/12] ACPI/IORT: Make iort_get_device_domain IRQ domain agnostic Lorenzo Pieralisi
2020-05-21 19:56   ` Bjorn Helgaas
2020-05-21 12:59 ` [PATCH 03/12] ACPI/IORT: Make iort_msi_map_rid() PCI agnostic Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 04/12] ACPI/IORT: Remove useless PCI bus walk Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 05/12] ACPI/IORT: Add an input ID to acpi_dma_configure() Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 06/12] of/iommu: Make of_map_rid() PCI agnostic Lorenzo Pieralisi
2020-05-21 22:47   ` Rob Herring
2020-06-04 14:27     ` Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 07/12] of/device: Add input id to of_dma_configure() Lorenzo Pieralisi
2020-05-21 23:02   ` Rob Herring
2020-06-04 14:49     ` Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 08/12] of/irq: make of_msi_map_get_device_domain() bus agnostic Lorenzo Pieralisi
2020-05-21 19:57   ` Bjorn Helgaas
2020-05-21 13:00 ` [PATCH 09/12] dt-bindings: arm: fsl: Add msi-map device-tree binding for fsl-mc bus Lorenzo Pieralisi
2020-05-21 23:10   ` Rob Herring
2020-05-22  9:42     ` Robin Murphy
2020-05-22  9:57       ` Diana Craciun OSS
2020-05-22 14:08         ` Rob Herring
2020-05-22 14:34           ` Robin Murphy [this message]
2020-05-22 14:02       ` Rob Herring
2020-05-22 15:38         ` Laurentiu Tudor
2020-05-21 13:00 ` [PATCH 10/12] of/irq: Make of_msi_map_rid() PCI bus agnostic Lorenzo Pieralisi
2020-05-21 23:17   ` Rob Herring
2020-06-04 15:08     ` Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 11/12] bus/fsl-mc: Refactor the MSI domain creation in the DPRC driver Lorenzo Pieralisi
2020-05-21 13:00 ` [PATCH 12/12] bus: fsl-mc: Add ACPI support for fsl-mc Lorenzo Pieralisi
2020-05-21 15:03   ` Laurentiu Tudor
2020-05-22  5:32     ` Makarand Pawagi
2020-05-22  9:53       ` Lorenzo Pieralisi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7f8d00ae-b225-a58d-8a11-e8c68edc877b@arm.com \
    --to=robin.murphy@arm.com \
    --cc=bhelgaas@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=diana.craciun@oss.nxp.com \
    --cc=guohanjun@huawei.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=makarand.pawagi@nxp.com \
    --cc=maz@kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git