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

From: Bharat Bhushan
Date: Wed Sep 27 2017 - 06:41:08 EST

Next message: Robin Murphy: "Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo"
Previous message: Ilya Matveychikov: "Re: [PATCH 4.4 03/26] lib/cmdline.c: fix get_options() overflow while parsing ranges"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Robin,

> -----Original Message-----
> From: linux-kernel-owner@xxxxxxxxxxxxxxx [mailto:linux-kernel-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Bharat Bhushan
> Sent: Wednesday, September 06, 2017 12:47 PM
> To: Robin Murphy <robin.murphy@xxxxxxx>; Marc Zyngier
> <marc.zyngier@xxxxxxx>; robh+dt@xxxxxxxxxx; Mark Rutland
> <mark.rutland@xxxxxxx>; will.deacon@xxxxxxx; oss@xxxxxxxxxxxx; Gang
> Liu <gang.liu@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> catalin.marinas@xxxxxxx
> Subject: RE: [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
>
> Hi Robin,
>
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@xxxxxxx]
> > Sent: Friday, September 01, 2017 4:29 PM
> > To: Bharat Bhushan <bharat.bhushan@xxxxxxx>; Marc Zyngier
> > <marc.zyngier@xxxxxxx>; robh+dt@xxxxxxxxxx; Mark Rutland
> > <mark.rutland@xxxxxxx>; will.deacon@xxxxxxx; oss@xxxxxxxxxxxx;
> Gang
> > Liu <gang.liu@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > catalin.marinas@xxxxxxx
> > Subject: Re: [PATCH] RM64: dts: ls208xa: Add iommu-map property for
> > pci
> >
> > On 01/09/17 11:13, Bharat Bhushan wrote:
> > >
> > >
> > >> -----Original Message----- From: linux-kernel-owner@xxxxxxxxxxxxxxx
> > >> [mailto:linux-kernel- owner@xxxxxxxxxxxxxxx] On Behalf Of Bharat
> > >> Bhushan Sent: Thursday, August 31, 2017 4:53 PM To: Marc Zyngier
> > >> <marc.zyngier@xxxxxxx>; robh+dt@xxxxxxxxxx; Mark Rutland
> > >> <mark.rutland@xxxxxxx>; will.deacon@xxxxxxx; oss@xxxxxxxxxxxx;
> > Gang
> > >> Liu <gang.liu@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > >> kernel@xxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx Subject: RE:
> > >> [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> > >>
> > >>
> > >>
> > >>> -----Original Message----- From: Marc Zyngier
> > >>> [mailto:marc.zyngier@xxxxxxx] Sent: Thursday, August 31, 2017
> > >>> 4:20 PM To: Bharat Bhushan <bharat.bhushan@xxxxxxx>;
> > >>> robh+dt@xxxxxxxxxx;
> > >> Mark
> > >>> Rutland <mark.rutland@xxxxxxx>; will.deacon@xxxxxxx;
> > >> oss@xxxxxxxxxxxx;
> > >>> Gang Liu <gang.liu@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> > >>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > >>> kernel@xxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx 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:
> > >>>>
> > >>>>> -----Original Message----- From: Marc Zyngier
> > >>>>> [mailto:marc.zyngier@xxxxxxx] Sent: Thursday, August 31, 2017
> > >>>>> 3:02 PM To: Bharat Bhushan <bharat.bhushan@xxxxxxx>;
> > >>>>> robh+dt@xxxxxxxxxx; ark.rutland@xxxxxxx; will.deacon@xxxxxxx;
> > >>>>> oss@xxxxxxxxxxxx; Gang
> > >>> Liu
> > >>>>> <gang.liu@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> > >>>>> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > >>>>> catalin.marinas@xxxxxxx Subject: Re: [PATCH] RM64: dts:
> > >>>>> ls208xa: Add iommu-map property for pci
> > >>>>>
> > >>>>> On 31/08/17 10:23, Bharat Bhushan wrote:
> > >>>>>> This patch adds iommu-map property for PCIe, which enables
> > SMMU
> > >>>>>> for these devices on LS208xA devices.
> > >>>>>>
> > >>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@xxxxxxx> ---
> > >>>>>> 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
> > >>>>>> is fixed-up by
> > >>>>> u-boot */
> > >>>>>
> > >>>>> What does this do when your version of u-boot doesn't fill this
> > >>>>> in for
> > >> you?
> > >>>>
> > >>>> 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.
> > >>
> > >>> 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

Below properties as suggested by you are correct from h/w behavior perspective

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

Are you ok with these changes?

Thanks
-Bharat

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

Next message: Robin Murphy: "Re: [PATCH 3/3] arm64: cpuinfo: display product info in /proc/cpuinfo"
Previous message: Ilya Matveychikov: "Re: [PATCH 4.4 03/26] lib/cmdline.c: fix get_options() overflow while parsing ranges"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]