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

[Robin Murphy]

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.