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>; (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. 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
