RM64: dts: ls208xa: Add iommu-map property for pci

Commit Message

Bharat Bhushan Aug. 31, 2017, 9:23 a.m. UTC

This patch adds iommu-map property for PCIe, which enables
SMMU for these devices on LS208xA devices.

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

Comments

Marc Zyngier Aug. 31, 2017, 9:32 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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?

Thanks,

	M.

Bharat Bhushan Aug. 31, 2017, 10:41 a.m. UTC | #2

> -----Original Message-----
> From: Marc Zyngier [mailto:[e-mail]]
> Sent: Thursday, August 31, 2017 3:02 PM
> To: Bharat Bhushan <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Gang Liu
> <[e-mail]>; [e-mail]; linux-arm-
> [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]>
> > ---
> >  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?

Thanks
-Bharat

> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

Marc Zyngier Aug. 31, 2017, 10:49 a.m. UTC | #3

[Fixing Mark's address...]

On 31/08/17 11:41, Bharat Bhushan wrote:
> 
>> -----Original Message-----
>> From: Marc Zyngier [mailto:[e-mail]]
>> Sent: Thursday, August 31, 2017 3:02 PM
>> To: Bharat Bhushan <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; Gang Liu
>> <[e-mail]>; [e-mail]; linux-arm-
>> [e-mail]; [e-mail];
>> [e-mail]
>> 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 <[e-mail]>
>>> ---
>>>  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?
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?

Thanks,
	
M.

Bharat Bhushan Aug. 31, 2017, 11:22 a.m. UTC | #4

> -----Original Message-----
> From: Marc Zyngier [mailto:[e-mail]]
> Sent: Thursday, August 31, 2017 4:20 PM
> To: Bharat Bhushan <[e-mail]>; [e-mail];
> Mark Rutland <[e-mail]>; [e-mail];
> [e-mail]; Gang Liu <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]
> 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:[e-mail]]
> >> Sent: Thursday, August 31, 2017 3:02 PM
> >> To: Bharat Bhushan <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; Gang
> Liu
> >> <[e-mail]>; [e-mail]; linux-arm-
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> 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 <[e-mail]>
> >>> ---
> >>>  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.

Will revert after confirming this.

Thanks
-Bharat
> 
> Thanks,
> 
> M.
> --
> Jazz is not dead. It just smells funny...

Mark Rutland Aug. 31, 2017, 11:30 a.m. UTC | #5

On Thu, Aug 31, 2017 at 11:49:37AM +0100, Marc Zyngier wrote:
> [Fixing Mark's address...]

Thanks!

> On 31/08/17 11:41, Bharat Bhushan wrote:
> > 
> >> -----Original Message-----
> >> From: Marc Zyngier [mailto:[e-mail]]
> >> Sent: Thursday, August 31, 2017 3:02 PM
> >> To: Bharat Bhushan <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; Gang Liu
> >> <[e-mail]>; [e-mail]; linux-arm-
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> 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 <[e-mail]>
> >>> ---
> >>>  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.

Indeed.

Either the property should be valid (and correctly representing the HW),
or it shouldn't be present. Relying on kernel implementation details is
*not* OK.

Thanks,
Mark.

Bharat Bhushan Sept. 1, 2017, 10:13 a.m. UTC | #6

> -----Original Message-----
> From: [e-mail] [mailto:linux-kernel-
> [e-mail]] On Behalf Of Bharat Bhushan
> Sent: Thursday, August 31, 2017 4:53 PM
> To: Marc Zyngier <[e-mail]>; [e-mail]; Mark
> Rutland <[e-mail]>; [e-mail];
> [e-mail]; Gang Liu <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]
> Subject: RE: [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> 
> 
> 
> > -----Original Message-----
> > From: Marc Zyngier [mailto:[e-mail]]
> > Sent: Thursday, August 31, 2017 4:20 PM
> > To: Bharat Bhushan <[e-mail]>; [e-mail];
> Mark
> > Rutland <[e-mail]>; [e-mail];
> [e-mail];
> > Gang Liu <[e-mail]>; [e-mail];
> > [e-mail]; linux- [e-mail];
> > [e-mail]
> > 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:[e-mail]]
> > >> Sent: Thursday, August 31, 2017 3:02 PM
> > >> To: Bharat Bhushan <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail]; Gang
> > Liu
> > >> <[e-mail]>; [e-mail]; linux-arm-
> > >> [e-mail]; [e-mail];
> > >> [e-mail]
> > >> 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 <[e-mail]>
> > >>> ---
> > >>>  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. 

Thanks
-Bharat

> 
> Will revert after confirming this.
> 
> Thanks
> -Bharat
> >
> > Thanks,
> >
> > M.
> > --
> > Jazz is not dead. It just smells funny...

Robin Murphy Sept. 1, 2017, 10:58 a.m. UTC | #7

On 01/09/17 11:13, Bharat Bhushan wrote:
> 
> 
>> -----Original Message----- From: [e-mail]
>> [mailto:linux-kernel- [e-mail]] On Behalf Of Bharat
>> Bhushan Sent: Thursday, August 31, 2017 4:53 PM To: Marc Zyngier
>> <[e-mail]>; [e-mail]; Mark Rutland
>> <[e-mail]>; [e-mail]; [e-mail]; Gang
>> Liu <[e-mail]>; [e-mail];
>> [e-mail]; linux- 
>> [e-mail]; [e-mail] Subject: RE:
>> [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
>> 
>> 
>> 
>>> -----Original Message----- From: Marc Zyngier
>>> [mailto:[e-mail]] Sent: Thursday, August 31, 2017
>>> 4:20 PM To: Bharat Bhushan <[e-mail]>;
>>> [e-mail];
>> Mark
>>> Rutland <[e-mail]>; [e-mail];
>> [e-mail];
>>> Gang Liu <[e-mail]>; [e-mail]; 
>>> [e-mail]; linux-
>>> [e-mail]; [e-mail] 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:[e-mail]] Sent: Thursday, August 31, 2017
>>>>> 3:02 PM To: Bharat Bhushan <[e-mail]>;
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; Gang
>>> Liu
>>>>> <[e-mail]>; [e-mail]; linux-arm- 
>>>>> [e-mail]; [e-mail]; 
>>>>> [e-mail] 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 <[e-mail]> --- 
>>>>>> 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.

Bharat Bhushan Sept. 6, 2017, 7:17 a.m. UTC | #8

Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:[e-mail]]
> Sent: Friday, September 01, 2017 4:29 PM
> To: Bharat Bhushan <[e-mail]>; Marc Zyngier
> <[e-mail]>; [e-mail]; Mark Rutland
> <[e-mail]>; [e-mail]; [e-mail]; Gang
> Liu <[e-mail]>; [e-mail]; linux-arm-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> 
> On 01/09/17 11:13, Bharat Bhushan wrote:
> >
> >
> >> -----Original Message----- From: [e-mail]
> >> [mailto:linux-kernel- [e-mail]] On Behalf Of Bharat
> >> Bhushan Sent: Thursday, August 31, 2017 4:53 PM To: Marc Zyngier
> >> <[e-mail]>; [e-mail]; Mark Rutland
> >> <[e-mail]>; [e-mail]; [e-mail];
> Gang
> >> Liu <[e-mail]>; [e-mail];
> >> [e-mail]; linux- [e-mail];
> >> [e-mail] Subject: RE:
> >> [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> >>
> >>
> >>
> >>> -----Original Message----- From: Marc Zyngier
> >>> [mailto:[e-mail]] Sent: Thursday, August 31, 2017
> >>> 4:20 PM To: Bharat Bhushan <[e-mail]>;
> >>> [e-mail];
> >> Mark
> >>> Rutland <[e-mail]>; [e-mail];
> >> [e-mail];
> >>> Gang Liu <[e-mail]>; [e-mail];
> >>> [e-mail]; linux- [e-mail];
> >>> [e-mail] 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:[e-mail]] Sent: Thursday, August 31, 2017
> >>>>> 3:02 PM To: Bharat Bhushan <[e-mail]>;
> >>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>> [e-mail]; Gang
> >>> Liu
> >>>>> <[e-mail]>; [e-mail]; linux-arm-
> >>>>> [e-mail]; [e-mail];
> >>>>> [e-mail] 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 <[e-mail]> ---
> >>>>>> 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

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.

Bharat Bhushan Sept. 27, 2017, 10:40 a.m. UTC | #9

Hi Robin,

> -----Original Message-----
> From: [e-mail] [mailto:linux-kernel-
> [e-mail]] On Behalf Of Bharat Bhushan
> Sent: Wednesday, September 06, 2017 12:47 PM
> To: Robin Murphy <[e-mail]>; Marc Zyngier
> <[e-mail]>; [e-mail]; Mark Rutland
> <[e-mail]>; [e-mail]; [e-mail]; Gang
> Liu <[e-mail]>; [e-mail]; linux-arm-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: RE: [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> 
> Hi Robin,
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:[e-mail]]
> > Sent: Friday, September 01, 2017 4:29 PM
> > To: Bharat Bhushan <[e-mail]>; Marc Zyngier
> > <[e-mail]>; [e-mail]; Mark Rutland
> > <[e-mail]>; [e-mail]; [e-mail];
> Gang
> > Liu <[e-mail]>; [e-mail]; linux-arm-
> > [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH] RM64: dts: ls208xa: Add iommu-map property for
> > pci
> >
> > On 01/09/17 11:13, Bharat Bhushan wrote:
> > >
> > >
> > >> -----Original Message----- From: [e-mail]
> > >> [mailto:linux-kernel- [e-mail]] On Behalf Of Bharat
> > >> Bhushan Sent: Thursday, August 31, 2017 4:53 PM To: Marc Zyngier
> > >> <[e-mail]>; [e-mail]; Mark Rutland
> > >> <[e-mail]>; [e-mail]; [e-mail];
> > Gang
> > >> Liu <[e-mail]>; [e-mail];
> > >> [e-mail]; linux-
> > >> [e-mail]; [e-mail] Subject: RE:
> > >> [PATCH] RM64: dts: ls208xa: Add iommu-map property for pci
> > >>
> > >>
> > >>
> > >>> -----Original Message----- From: Marc Zyngier
> > >>> [mailto:[e-mail]] Sent: Thursday, August 31, 2017
> > >>> 4:20 PM To: Bharat Bhushan <[e-mail]>;
> > >>> [e-mail];
> > >> Mark
> > >>> Rutland <[e-mail]>; [e-mail];
> > >> [e-mail];
> > >>> Gang Liu <[e-mail]>; [e-mail];
> > >>> [e-mail]; linux-
> > >>> [e-mail]; [e-mail] 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:[e-mail]] Sent: Thursday, August 31, 2017
> > >>>>> 3:02 PM To: Bharat Bhushan <[e-mail]>;
> > >>>>> [e-mail]; [e-mail]; [e-mail];
> > >>>>> [e-mail]; Gang
> > >>> Liu
> > >>>>> <[e-mail]>; [e-mail]; linux-arm-
> > >>>>> [e-mail]; [e-mail];
> > >>>>> [e-mail] 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 <[e-mail]> ---
> > >>>>>> 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.

Patch

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 */
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 0 0 109 4>,
@@ -627,6 +628,7 @@ 
 			num-lanes = <4>;
 			bus-range = <0x0 0xff>;
 			msi-parent = <&its>;
+			iommu-map = <0 &smmu 0 1>;	/* This is fixed-up by u-boot */
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 0 0 114 4>,
@@ -648,6 +650,7 @@ 
 			num-lanes = <8>;
 			bus-range = <0x0 0xff>;
 			msi-parent = <&its>;
+			iommu-map = <0 &smmu 0 1>;	/* This is fixed-up by u-boot */
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 0 0 119 4>,
@@ -669,6 +672,7 @@ 
 			num-lanes = <4>;
 			bus-range = <0x0 0xff>;
 			msi-parent = <&its>;
+			iommu-map = <0 &smmu 0 1>;	/* This is fixed-up by u-boot */
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0000 0 0 1 &gic 0 0 0 124 4>,