[PATCH] RM64: dts: ls208xa: Add iommu-map property for pci

Hi Robin,

-----Original Message-----
From: Robin Murphy [mailto:robin.murphy at arm.com]
Sent: Friday, September 01, 2017 4:29 PM
To: Bharat Bhushan <redacted>; Marc Zyngier
[off-list ref]; robh+dt at kernel.org; Mark Rutland
[off-list ref]; will.deacon at arm.com; oss at buserror.net; Gang
Liu [off-list ref]; devicetree at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
catalin.marinas at arm.com
Subject: Re: [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci

On 01/09/17 11:13, Bharat Bhushan wrote:

quoted

quoted

-----Original Message----- From: linux-kernel-owner at vger.kernel.org
[mailto:linux-kernel- owner at vger.kernel.org] On Behalf Of Bharat
Bhushan Sent: Thursday, August 31, 2017 4:53 PM To: Marc Zyngier
[off-list ref]; robh+dt at kernel.org; Mark Rutland
[off-list ref]; will.deacon at arm.com; oss at buserror.net;

Gang

quoted

quoted

Liu [off-list ref]; devicetree at vger.kernel.org;
linux-arm-kernel at lists.infradead.org; linux- kernel at vger.kernel.org;
catalin.marinas at arm.com Subject: RE:
[PATCH] RM64: dts: ls208xa: Add iommu-map property for pci

quoted

-----Original Message----- From: Marc Zyngier
[mailto:marc.zyngier at arm.com] Sent: Thursday, August 31, 2017
4:20 PM To: Bharat Bhushan [off-list ref];
robh+dt at kernel.org;

Mark

quoted

Rutland [off-list ref]; will.deacon at arm.com;

oss at buserror.net;

quoted

Gang Liu [off-list ref]; devicetree at vger.kernel.org;
linux-arm-kernel at lists.infradead.org; linux- kernel at vger.kernel.org;
catalin.marinas at arm.com Subject: Re:
[PATCH] RM64: dts: ls208xa: Add iommu-map property for pci

[Fixing Mark's address...]

On 31/08/17 11:41, Bharat Bhushan wrote:

quoted

quoted

-----Original Message----- From: Marc Zyngier
[mailto:marc.zyngier at arm.com] Sent: Thursday, August 31, 2017
3:02 PM To: Bharat Bhushan [off-list ref];
robh+dt at kernel.org; ark.rutland at arm.com; will.deacon at arm.com;
oss at buserror.net; Gang

Liu

quoted

quoted

[off-list ref]; devicetree at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
catalin.marinas at arm.com Subject: Re: [PATCH] RM64: dts:
ls208xa: Add iommu-map property for pci

On 31/08/17 10:23, Bharat Bhushan wrote:

quoted

This patch adds iommu-map property for PCIe, which enables

SMMU

quoted

quoted

quoted

quoted

quoted

quoted

for these devices on LS208xA devices.

Signed-off-by: Bharat Bhushan <redacted> ---
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ++++ 1 file
changed, 4 insertions(+)

diff --git
a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi index
94cdd30..67cf605 100644 ---
a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi +++
b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi @@ -606,6
+606,7 @@ num-lanes = <4>; bus-range = <0x0 0xff>;
msi-parent = <&its>; +			iommu-map = <0 &smmu 0

1>;	/* This

quoted

quoted

quoted

quoted

quoted

quoted

is fixed-up by

u-boot */

What does this do when your version of u-boot doesn't fill this in
for

you?

quoted

quoted

Good question, frankly I have not thought of this case before.
But if we pass length = 0 in above property then no fixup happen
with happen with older u-boot. In this case
of_iommu_configure() will return NULL iommu-ops and it switch to
swio-tlb. Will that work?

I really don't like this. You rely on having invalid data in the DT,
and that seems just wrong.

Why can't u-boot just generate that property, and we leave the DT
alone?

We do not have smmu phandle allowing uboot to generate this.

quoted

Or even better, you provide the right information for the few boards
that are based on this SoC, not relying on u-boot for anything that
is in the kernel tree?

On our platforms we have a h/w table which converts RID->Device-Id.
I will check what will happen if that table is not initialized, can
RID be equal to device-id is that case. If that will be allowed than
we can give right information that will work with u-boot not updating
this property.

U-boot uses a stream-id allocator and programs the h/w mapping table
(rid to sid mapping table). Also it updates iommu-map property
accordingly. But If u-boot does not update iommu-map than we cannot
have a valid full proof solution as stream-id allocation can change in
u-boot.

So the other option of u-boot generating this entry seems correct
solution. This requires u-boot to know iommu-phandle, something
similar to "msi-parent" used for "msi-map" Device-tree binding need
change to add iommu-phandle/iommu-parent for this.

From what I know of this hardware, it's going to be rather difficult to concoct
a DT which reflects the initial hardware state accurately *and* works
correctly without updating u-boot in lockstep. IIRC, I believe the accurate
description for an unprogrammed LUT would be "everything comes out on
the default ID, which defaults to 0", i.e.:

	iommu-map-mask = <0x0>;
	iommu-map = <0x0 &smmu 0x0 0x1>;

These changes are not enough to make PCI devise working with LUT disabled, also needed msi-map maps all requester-id to "0", using "msi-map-mask = 0x0".

Why we assume that no "msi-map" property means untranslated requested-id, why not consider that translated to "0".

(assuming the SMMU has stream-id-mask set appropriately too)

That's OK except if u-boot updates the map without removing the mask,
whereupon things will go wrong, and I guess that would be the case with
current u-boot :(

However, the existing MSI description is already bogus - if u-boot didn't
program the LUT, the ITS driver would treat the invalid "msi-parent" property
as this:

	msi-map = <0x0 &its 0x0 0xffff>;

which means that nobody other than 0:0.0 has working MSIs anyway.

We can have following version of u-boot:
 1) No LUT setup, does not generate msi-map and does not update/generate iommu-map (older u-boot)

     For this case the working device tree changes can be:
                       iommu-map-mask = <0>;
                       iommu-map = <0x0 &smmu 0 0x1>; 
                       msi-map-mask = <0x0>;
                       msi-map = <0x0 &its 0 0x1>;

     But these changes will not work with current version of u-boot (below (2))

2) LUT setup and msi-map generated but no iommu-map generated (current case)

    I do not see we can have a working device tree for this.
    Probably generating iommu-map in u-boot is better, with that msi-map and iommu-map will be consistent (below (4))

3) LUT setup, "msi-map" generated and iommu-map updated

      We can have below change is device tree.
       	         iommu-map = <0x0 &smmu 0 0x1>;

       But this change will not work with (1) and (2) above.

4) LUT setup, "msi-map" generated and iommu-map also generated by u-boot.
    There is no iommu-map entry needed in device tree but u-boot need to know iommu phandle.
    While iommus is supposed to be used in iommu-node and not in pci node.

Looking for suggestion

Thanks
-Bharat

If you want an obviously-invalid placeholder equivalent to the use of "msi-
parent" then I'd suggest just:

	iommus = <&smmu>;

which would be ignored by Linux for PCI devices, so provided you didn't
disable bypass at the SMMU things might in theory still work in the "u-boot
does nothing" case. Otherwise, the implied identity map is probably slightly
preferable to the unit-length map, i.e.:

	iommu-map = <0x0 &smmu 0x0 0xffff>;

which is at least equally broken as MSIs in the same situation.

Robin.