Re: [RFC PATCH 1/2] dt-bindings: pci: Add drop mask property for MSI and IOMMU

[Robin Murphy]

On 07/07/17 14:30, Mark Rutland wrote:
> On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
>> This patch adds info about optional DT properties
>> iommu-map-drop-mask and msi-map-drop-mask.
>>
>> A drop mask represents the bits which will be
>> removed/dropped by system from Requester ID before
>> mapping it to msi ID or stream ID.
>>
>> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxxxx>
>> Signed-off-by: Oza Pawandeep <oza.oza@xxxxxxxxxxxx>
>> Signed-off-by: Srinath Mannam <srinath.mannam@xxxxxxxxxxxx>
>> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/pci/pci-iommu.txt          | 31 ++++++++++++++++++++
>>  Documentation/devicetree/bindings/pci/pci-msi.txt  | 33 ++++++++++++++++++++++
>>  2 files changed, 64 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> index 0def586..499cb27 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> @@ -44,6 +44,9 @@ Optional properties
>>  - iommu-map-mask: A mask to be applied to each Requester ID prior to being
>>    mapped to an IOMMU specifier per the iommu-map property.
>>  
>> +- iommu-map-drop-mask: A drop mask represents the bits which will be
>> +  removed/dropped by system from Requester ID before mapping it to
>> +  stream ID.
> 
> As mentioned in my reply to the cover letter, you're expecting this to
> be handled as more than a mask, so this description is inadequate.
> 
> [...]
> 
>> +/ {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	iommu: iommu@a {
>> +		reg = <0xa 0x1>;
>> +		compatible = "vendor,some-iommu";
>> +		#iommu-cells = <1>;
>> +	};
>> +
>> +	pci: pci@f {
>> +		reg = <0xf 0x1>;
>> +		compatible = "vendor,pcie-root-complex";
>> +		device_type = "pci";
>> +
>> +		/*
>> +		 * The sideband data provided to the IOMMU is a 10bit
>> +		 * data derived from the RID by dropping 4 MSBs
>> +		 * of device number and 2 MSBs of function number.
>> +		 */
>> +		iommu-map = <0x0 &iommu 0x0 0x1024>;
>> +		iommu-map-drop-mask = <0xff09>;
>> +	};
>> +};
> 
> ... as this this example.
> 
> Assuming this was truly a mask of bits to drop, you'd have:
> 
> 	RID	->	SID
> 	0xffff	->	0x00f6
> 
> ... whereas from your cover letter it seems you want:
> 
> 	RID	->	SID
> 	0xffff	->	0x3f
> 
> ... and I've just realsied you have non-coniguous masks, so this is even
> worse.
> 
>> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
>> index 9b3cc81..1de3f39 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
>> @@ -49,6 +49,10 @@ Optional properties
>>  - msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
>>    to an msi-specifier per the msi-map property.
>>  
>> +- msi-map-drop-mask: A drop mask represents the bits which will be
>> +  removed/dropped by system from Requester ID before mapping it to
>> +  msi ID.
>> +
>>  - msi-parent: Describes the MSI parent of the root complex itself. Where
>>    the root complex and MSI controller do not pass sideband data with MSI
>>    writes, this property may be used to describe the MSI controller(s)
>> @@ -218,3 +222,32 @@ Example (5)
>>  			  <0x0000 &msi_b 0x0000 0x10000>;
>>  	};
>>  };
>> +
>> +Example (6)
>> +===========
>> +
>> +/ {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +
>> +	msi: msi-controller@a {
>> +		reg = <0xa 0x1>;
>> +		compatible = "vendor,some-controller";
>> +		msi-controller;
>> +		#msi-cells = <1>;
>> +	};
>> +
>> +	pci: pci@f {
>> +		reg = <0xf 0x1>;
>> +		compatible = "vendor,pcie-root-complex";
>> +		device_type = "pci";
>> +
>> +		/*
>> +		 * The sideband data provided to the  MSI controller is
>> +		 * a 10bit data derived from the RID by dropping
>> +		 * 4 MSBs of device number and 2 MSBs of function number.
>> +		 */
>> +		msi-map = <0x0 &msi_a 0x0 0x100>,
>> +		msi-map-drop-mask = <0xff09>
>> +	};
>> +};
> 
> ... likewise on all counts.
> 
> Your mapping can be expressed today using a number of msi-map entries,
> which you can easily generate programmatically with a trivial perl
> script, without requiring a new binding or any new kernel code.
> 
> Please do that instead.

Indeed. The systems I'm aware of which need to express non-trivial RID
to SID mappings tend to have the bootloader probe PCI and dynamically
generate map entries per discovered RID, but even if you wanted to
statically generate the whole lot for the worst-case bus range that's
still only 512 entries, which is not unmanageable. Notably, it's also
what would have to be done (in equivalent) for IORT, although I assume
this is an embedded platform for which nobody cares about ACPI.

Robin.